-
Notifications
You must be signed in to change notification settings - Fork 643
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[OIDC] Add V5 API key format for short lived API keys #10346
base: dev
Are you sure you want to change the base?
Conversation
return false; | ||
} | ||
|
||
if (plaintextApiKey.Substring(52, 6) != IdentifiableSecrets.CommonAnnotatedKeySignature) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create constants for offsets (and lengths maybe?)
} | ||
|
||
char type = plaintextApiKey[72]; | ||
if (!TryParseBase62Char(type, out _)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are potentially out of range values for environment
and type
purposefully ignored?
} | ||
|
||
|
||
// ensure all but the 4 least significant bytes are 0 (little-endian) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a function, especially since it is done twice in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...or LINQ expression better capturing the intent: expirationBytes.Take(expirationBytes.Length - 4).All(0)
.
// We encode the expiration as a base62 string. The integer we encode is the total minutes of the expiration divided by 5. | ||
// We can only use 3 ASCII characters to encode the expiration, so the integer value must be less than 62^3. We could support | ||
// up to about 827 days but our long lived API keys only last up to a year so that's not needed. | ||
if (expiration > TimeSpan.FromDays(366)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we not have max expiration constant in the code somewhere? Or does it only exist in UI?
{ | ||
throw new ArgumentOutOfRangeException(nameof(expiration), $"The {nameof(expiration)} must be 366 days or shorter."); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check that expiration
is non-negative?
} | ||
|
||
[Fact] | ||
public void RejectsInvalidString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think more "invalid string" tests could be made. On top of my head:
- Correct key length, correct checksum, but non-base62 characters in the string (might need separate variants for each field);
- Correct checksum, but incorrect key length.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, could use less magic numbers.
This uses the standard API key format provided by HISv2 (in the Microsoft.Security.Utilites.Core package) and adds a new format for short-lived API keys.
See the code comments for information on the format.
This will be used by the OIDC flow for short-lived API keys.