Skip to content

Require 4/22 ice-chars for local ICE credentials - #949

Merged
fippo merged 1 commit into
pion:mainfrom
fippo:ice-ufrag-pwd-length
Aug 4, 2026
Merged

Require 4/22 ice-chars for local ICE credentials#949
fippo merged 1 commit into
pion:mainfrom
fippo:ice-ufrag-pwd-length

Conversation

@fippo

@fippo fippo commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

An ice-char carries 6 bits, not 8, so the RFC 8445 randomness
minimums are exactly the RFC 8839 grammar minimums of 4 and 22
characters. The old *8 check accepted a 16-char password carrying
~96 bits against a 128-bit requirement, and values too short to
be legal in SDP.

RFC 5245 description is still the best:
https://datatracker.ietf.org/doc/html/rfc5245#section-15.4

An ice-char carries 6 bits, not 8, so the RFC 8445 randomness
minimums are exactly the RFC 8839 grammar minimums of 4 and 22
characters. The old *8 check accepted a 16-char password carrying
~96 bits against a 128-bit requirement, and values too short to
be legal in SDP.

RFC 5245 description is still the best:
 https://datatracker.ietf.org/doc/html/rfc5245#section-15.4
@fippo
fippo force-pushed the ice-ufrag-pwd-length branch from 9f40dee to 5921cd2 Compare August 4, 2026 13:46
@codecov

codecov Bot commented Aug 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.24%. Comparing base (6714a32) to head (5921cd2).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #949      +/-   ##
==========================================
- Coverage   88.27%   88.24%   -0.03%     
==========================================
  Files          46       46              
  Lines        6277     6279       +2     
==========================================
  Hits         5541     5541              
- Misses        501      505       +4     
+ Partials      235      233       -2     
Flag Coverage Δ
go 88.24% <100.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JoTurk JoTurk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch :)

@fippo

fippo commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

asked Claude about something else, it blurbed too much in response and said something that sounded wrong ;-)

@fippo
fippo merged commit c0c488f into pion:main Aug 4, 2026
18 checks passed
@JoTurk

JoTurk commented Aug 4, 2026

Copy link
Copy Markdown
Member

@fippo should we also validate that it's valid ice-chars? i added a helper a while ago while i was implementing the extensions parser

ice/candidate_base.go

Lines 1003 to 1026 in c0c488f

// Read an ice-char token from the raw string
// ice-char = ALPHA / DIGIT / "+" / "/"
// stop reading when a space is encountered or the end of the string.
func readCandidateCharToken(raw string, start int, limit int) (string, int, error) { //nolint:cyclop
for i, char := range raw[start:] {
if char == 0x20 { // SP
return raw[start : start+i], start + i + 1, nil
}
if i == limit {
//nolint: err113 // handled by caller
return "", 0, fmt.Errorf("token too long: %s expected 1x%d", raw[start:start+i], limit)
}
if (char < 'A' || char > 'Z') &&
(char < 'a' || char > 'z') &&
(char < '0' || char > '9') &&
char != '+' && char != '/' {
return "", 0, fmt.Errorf("invalid ice-char token: %c", char) //nolint: err113 // handled by caller
}
}
return raw[start:], len(raw), nil
}

@fippo
fippo deleted the ice-ufrag-pwd-length branch August 4, 2026 15:37
@fippo

fippo commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

for remote that does not work since a lot of people insist on base64. We might for local which we control

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants