Relax format restrictions on authentication tokens #1978
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
inahga
approved these changes
Sep 25, 2023
core/src/task.rs
Outdated
| @@ -632,30 +632,30 @@ pub enum AuthenticationToken { | |||
| /// [2]: https://datatracker.ietf.org/doc/html/draft-dcook-ppm-dap-interop-test-design-03#section-4.3.3 | |||
| /// [3]: https://datatracker.ietf.org/doc/html/draft-dcook-ppm-dap-interop-test-design-03#section-4.4.2 | |||
| /// [4]: https://datatracker.ietf.org/doc/html/draft-ietf-ppm-dap-01#name-https-sender-authentication | |||
| DapAuth(TokenInner), | |||
| DapAuth(DapAuthTokenInner), | |||
There was a problem hiding this comment.
Consider just having these be BearerToken and DapAuthToken. The Inner suffix suggests that there's some different public opaque outer type, which is not the case here.
divergentdave
force-pushed
the
david/relax-auth-token
branch
from
9cb2433 to
1d0eab4
Compare
tgeoghegan
approved these changes
Sep 25, 2023
core/src/task.rs
Outdated
| /// Base64 alphabet, as specified in [RFC 4648 section 5][1]. The unpadded, URL-safe Base64 string | ||
| /// is the canonical form of the token and is used in configuration files, Janus aggregator API | ||
| /// requests and HTTP authentication headers. | ||
| /// This token is used directly in HTTP request headers without further encoding and so much be a |
There was a problem hiding this comment.
Suggested change
| /// This token is used directly in HTTP request headers without further encoding and so much be a | |
| /// This token is used directly in HTTP request headers without further encoding and so must be a |
divergentdave
force-pushed
the
david/relax-auth-token
branch
from
1d0eab4 to
4ec61c0
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This relaxes the requirements we place on authentication tokens. The last time we changed this, we required that all
DAP-Auth-TokenandAuthorization: Bearervalues were in unpadded base64url. This causes problems when using the interop test API outside of Janus's integration tests, because it's more restrictive than it used to be. In one case, I was using structured values for these, consisting of a word describing the token's role, a hyphen, and then some random hexadecimal bytes. This failed the base64url check in one case because it had an impossible length. (base64 cannot produce an output with length one more than a multiple of four, ignoring padding)This PR places different, looser, validation requirements on each kind of authentication token. For
DAP-Auth-Token, we require that the value be a valid HTTP header value. (i.e. we exclude some nonprintable control characters) Unlike in older versions, we also require that the value be valid UTF-8. (this makes implementation simpler, and there's no need to go out of our way supporting invalid UTF-8 inside a HTTP payload) ForAuthorization: Bearer, we check against the language described in RFC 6750. In addition to unpadded base64url, we will now support other base64 variants, compact JWT, and other formats that fit in the expanded alphabet. For both kinds of authentication tokens, we still generate their values internally by base64url-encoding random bytes.This change ought to go in now, during our pre-release window, because it makes breaking changes to variants of a public
enum.