-
Notifications
You must be signed in to change notification settings - Fork 265
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
DPoP Support #1495
DPoP Support #1495
Conversation
ef2e4af
to
a874973
Compare
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 see a lot of different casing of "dpop" both in variables and in header names. In general I think the headers might be simplified if we always set them to be lowercase, or at least expect them in lowercase when reading values since headers are supposed to case-insensitive.
Are you tired of typing "DPoP" yet?
lib/errors/WWWAuthError.ts
Outdated
// example string: Bearer error="invalid_token", error_description="The access token is invalid" | ||
// regex will match on `error="invalid_token", error_description="The access token is invalid"` | ||
// see unit test for more examples of possible www-authenticate values | ||
const regex = /(?:,|, )?([a-zA-Z0-9!#$%&'*+-.^_`|~]+)=(?:"([a-zA-Z0-9!#$%&'*+-.^_`|~ /:]+)"|([0-9]+))/g; |
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.
Missing escape on forward slash. You could also name the capture groups so 1,2,3 below are not so esoteric
const regex = /(?:,|, )?([a-zA-Z0-9!#$%&'*+-.^_`|~]+)=(?:"([a-zA-Z0-9!#$%&'*+-.^_`|~ /:]+)"|([0-9]+))/g; | |
const regex = /(?:,|, )?([a-zA-Z0-9!#$%&'*+-.^_`|~]+)=(?:"([a-zA-Z0-9!#$%&'*+-.^_`|~ \/:]+)"|([0-9]+))/g; |
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.
yarn run v1.22.19
$ eslint --ext .js,.ts,.jsx .
/Users/jared/Code/devex/auth-js/lib/errors/WWWAuthError.ts
62:90 error Unnecessary escape character: \/ no-useless-escape
✖ 1 problem (1 error, 0 warnings)
error Command failed with exit code 1.
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 it doesn't need escaping because it's in a [ ]
// else { | ||
// // WWWAuthError.parseHeader may return null, only overwrite if !null | ||
// err = wwwAuthErr ?? err; | ||
// } |
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.
Is this condition reachable?
// throws error is dpop-nonce header cannot be found, prevents infinite loop | ||
throw new AuthApiError( | ||
{errorSummary: 'No `dpop-nonce` header found when required'}, | ||
err.resp ?? undefined // yay ts |
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.
lol
refreshToken: RefreshToken | ||
): Promise<OAuthResponse> { | ||
return httpRequest(sdk, { | ||
const data = Object.entries({ |
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.
Not sure what you discussed offline. Did you consider URLSearchParams()?
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 did not. This is function was already written and needs to run on Node14, I believe URLSearchParam
would require a polyfill for node
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.
Not insisting, but it looks like URLSearchParams
is supported since [email protected]
@@ -36,11 +36,23 @@ export async function getUserInfo<T extends CustomUserClaims = CustomUserClaims> | |||
return Promise.reject(new AuthSdkError('getUserInfo requires an ID token object')); | |||
} | |||
|
|||
return httpRequest(sdk, { | |||
const options: any = { |
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.
nit(ts): Does type inference complain because you delete
a property below? Couldn't you add it in generateDPoPProof({...options, accessToken: accessTokenObject.accessToken, keyPair})
to avoid that?
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.
TS never complained about the usage of delete
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.
The delete
syntax wasn't the problem I was pointing out. I was saying that removing the property causes TS complain that the types no longer match.
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 haven't seen a TS error from this line. httpOptions defines accessToken
as an optional prop
#### Handling `use_dpop_nonce` | ||
<sub><sup>*Reference: **Resource Server-Provided Nonce** ([RFC9449](https://datatracker.ietf.org/doc/html/rfc9449#name-resource-server-provided-no))*</sub></sup> | ||
|
||
> Resource servers can also choose to provide a nonce value to be included in DPoP proofs sent to them. They provide the nonce using the DPoP-Nonce header in the same way that authorization servers do... |
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.
nit: It would be more precise to say that the resource server is requiring a nonce (as the error_description describes) rather than providing.
a29daa9
to
5e8bd34
Compare
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.
All the MixedCasingOfDPoP reminds me of AIM. I've reviewed for content and consistency, but I'm not an expert in dpop or plan w.r.t. SDK, so you should probably wait for @mikenachbaur-okta 's review
refreshToken: RefreshToken | ||
): Promise<OAuthResponse> { | ||
return httpRequest(sdk, { | ||
const data = Object.entries({ |
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.
Not insisting, but it looks like URLSearchParams
is supported since [email protected]
@@ -36,11 +36,23 @@ export async function getUserInfo<T extends CustomUserClaims = CustomUserClaims> | |||
return Promise.reject(new AuthSdkError('getUserInfo requires an ID token object')); | |||
} | |||
|
|||
return httpRequest(sdk, { | |||
const options: any = { |
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.
The delete
syntax wasn't the problem I was pointing out. I was saying that removing the property causes TS complain that the types no longer match.
// widget-based tests cannot be added until new authjs client is updated in siw | ||
// xit('can login using signin widget (no redirect)', async () => { | ||
// await bootstrap(); | ||
// await loginWidget(flow); | ||
// await TestApp.getUserInfo(); | ||
// await TestApp.assertUserInfo(); | ||
// await TestApp.logoutRedirect(); | ||
// await assertNoRemainingDPoPKeys(); | ||
// }); | ||
|
||
// // widget-based tests cannot be added until new authjs client is updated in siw | ||
// xit('can login using signin widget (with redirect)', async () => { | ||
// let options = { forceRedirect: true }; | ||
// await bootstrap(options); | ||
// await loginWidget(flow, true); | ||
// await TestApp.getUserInfo(); | ||
// await TestApp.assertUserInfo(); | ||
// await TestApp.logoutRedirect(); | ||
// await assertNoRemainingDPoPKeys(); | ||
// }); |
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 these belong in @okta/okta-signin-widget
anyway?
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.
perhaps, they already exist in specs/login
, which is what I based this file on
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.
might be best to use the downstream okta-auth-js → okta-signin-widget instead of duplicating?
jest.mock('../../../lib/oidc/dpop', () => { | ||
const actual = jest.requireActual('../../../lib/oidc/dpop'); | ||
return { | ||
__esModule: true, | ||
...actual, | ||
...mocked | ||
}; | ||
}); |
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.
interesting. any notes?
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.
Any note in particular you're looking for? I want to DPoP methods to be tested, but db wrappers (like findKeyPair) can be mocked
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.
guess not. unusual syntax so i was just wondering if there's anything to watch out for
expect(mocked.clearDPoPKeyPair).toHaveBeenCalledWith('foo'); | ||
expect(mocked.clearAllDPoPKeyPairs).not.toHaveBeenCalled(); |
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.
is this the new assertion based on my previous question?
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.
Yea
.mockRejectedValueOnce(new OAuthError('use_dpop_nonce', | ||
'Authorization server requires nonce in DPoP proof.', | ||
{ status: 400, responseText: 'Bad Request', headers: { 'dpop-nonce': 'nonceuponatime' }}) |
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.
lol
generateDPoPProof.mockResolvedValue('dpopproofyay'); | ||
const httpSpy = jest.spyOn(http, 'httpRequest').mockResolvedValue({ | ||
'sub': tokens.standardIdTokenParsed.claims.sub, | ||
'email': '[email protected]', |
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.
lol
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 can't take credit for saml jackson, he's a test user
'prompt': 'none' | ||
} | ||
}, | ||
time: 1449699929, |
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.
Is the hardcoded timestamp ok?
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.
Yea
it('throws if dpop=true and token_type is not DPoP', async () => { | ||
let errorThrown = false; | ||
try { | ||
await handleOAuthResponse(sdk, { responseType: ['token', 'id_token'], dpop: true }, { }, undefined as unknown as CustomUrls); |
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.
What's the double as
for?
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.
Copied from test(s) above. Doesn't seem to have any affect
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've run into this before where TS complains about a direct cast between "incompatible" types, so you have to cast it to any
or unknown
in between. 🤷🏼♀️
// regex will match on `error="invalid_token", error_description="The access token is invalid"` | ||
// see unit test for more examples of possible www-authenticate values | ||
// eslint-disable-next-line max-len | ||
const regex = /(?:,|, )?([a-zA-Z0-9!#$%&'*+\-.^_`|~]+)=(?:"([a-zA-Z0-9!#$%&'*+\-.,^_`|~ /:]+)"|([a-zA-Z0-9!#$%&'*+\-.^_`|~/:]+))/g; |
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.
const regex = /(?:,|, )?([a-zA-Z0-9!#$%&'*+\-.^_`|~]+)=(?:"([a-zA-Z0-9!#$%&'*+\-.,^_`|~ /:]+)"|([a-zA-Z0-9!#$%&'*+\-.^_`|~/:]+))/g; | |
const regex = /(?:,|, )?([a-zA-Z0-9!#$%&'*+\-.^_`|~]+)=(?:"([a-zA-Z0-9!#$%&'*+\-.,^_`|~ \/:]+)"|([a-zA-Z0-9!#$%&'*+\-.^_`|~\/:]+))/g; |
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.
yarn run v1.22.19
$ eslint --ext .js,.ts,.jsx .
/Users/jared/Code/devex/auth-js/lib/errors/WWWAuthError.ts
62:90 error Unnecessary escape character: \/ no-useless-escape
✖ 1 problem (1 error, 0 warnings)
error Command failed with exit code 1.
I think because the character is inside a [ ]
it doesn't need escaping (where -
does)
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.
Let me guess, it doesn't use PCRE but some weird variation? 🤷🏼♀️
const signature = await webcrypto.subtle.sign( | ||
{ name: signingKey.algorithm.name }, signingKey, stringToBuffer(`${head}.${body}`) | ||
); | ||
return `${head}.${body}.${base64ToBase64Url(bufferToBase64Url(signature))}`; |
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 like function bufferToBase64Url
should be actually named bufferToBase64
(as it can contain Base64 padding) ?
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.
Does seem that way, but I think it's a bit out-of-scope to rename it
https://github.com/okta/okta-auth-js/blob/master/lib/crypto/base64.ts#L72
const hash = await webcrypto.subtle.digest('SHA-256', buffer); | ||
|
||
return btoa(String.fromCharCode.apply(null, new Uint8Array(hash) as unknown as number[])) | ||
.replace(/\+/g, '-').replace(/\//g, '_').replace(/=+$/, ''); |
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.
Use bufferToBase64Url
and base64ToBase64Url
utils?
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.
c5aa7df#diff-a500248e65a86922110daa0dda780cdf1def9b2a13bff7c02c25243d3e312b79R215
I originally had this, but it wasn't working so I wrote the hashAccessToken
function. I now see my mistake, I used stringToBuffer
AND stringToBase64Url
🤦
const { accessToken, refreshToken } = tokens; | ||
|
||
// revoking access token and refresh token doesn't exist | ||
if (revokedToken === 'access' && accessToken && accessToken.tokenType === 'DPoP' && !refreshToken) { |
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.
IMHO there's a potential naming confusion between token type
okta-auth-js/lib/oidc/types/Token.ts
Line 25 in 4dc55e9
tokenType: string; |
and token kind
okta-auth-js/lib/oidc/types/Token.ts
Line 45 in 4dc55e9
export type TokenType = 'accessToken' | 'idToken' | 'refreshToken'; |
Maybe add comment to line 25 about possible values (
Bearer
or DPoP
)?
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.
Also, theoretically, the token type is dictated by the .../token
endpoint's token_type
value. So when populating the authorization header, it should use
Authorization: ${token.token_type} ${token.access_token}
Other types of tokens returned from the server could indicate other token types, but according to the RFC the value must be DPoP. So I think this is safe here.
lib/oidc/dpop.ts
Outdated
|
||
/////////// crypto /////////// | ||
|
||
export async function writeJwt(header: object, claims: object, signingKey: CryptoKey): Promise<string> { |
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.
nit: Maybe change the word write
to create
? As this function doesn't write but returns
lib/oidc/getUserInfo.ts
Outdated
const keyPair = await findKeyPair(accessTokenObject.dpopPairId); | ||
const proof = await generateDPoPProof({ ...options, keyPair }); | ||
options.headers = { |
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.
Reuse getDPoPAuthorizationHeaders
util?
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.
Agreed, is there any reason why the util can't be reused here?
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 have some comments, but overall it looks great
// regex will match on `error="invalid_token", error_description="The access token is invalid"` | ||
// see unit test for more examples of possible www-authenticate values | ||
// eslint-disable-next-line max-len | ||
const regex = /(?:,|, )?([a-zA-Z0-9!#$%&'*+\-.^_`|~]+)=(?:"([a-zA-Z0-9!#$%&'*+\-.,^_`|~ /:]+)"|([a-zA-Z0-9!#$%&'*+\-.^_`|~/:]+))/g; |
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.
Let me guess, it doesn't use PCRE but some weird variation? 🤷🏼♀️
const algorithm = { | ||
name: 'RSASSA-PKCS1-v1_5', | ||
hash: 'SHA-256', | ||
modulusLength: 2048, | ||
publicExponent: new Uint8Array([0x01, 0x00, 0x01]), | ||
}; |
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 need to respect the signing algorithms advertised from the openid-configuration
endpoint's dpop_signing_alg_values_supported
values? Or we might need to add a note or issue to the backlog to acknowledge that we only work with SHA256.
lib/oidc/dpop.ts
Outdated
|
||
// generates a crypto (non-extractable) private key pair and writes it to indexeddb, returns key (id) | ||
export async function createDPoPKeyPair (): Promise<{keyPair: CryptoKeyPair, keyPairId: string}> { | ||
// TODO: clear exist keys when creating new one? |
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.
Is this comment still relevant?
const { accessToken, refreshToken } = tokens; | ||
|
||
// revoking access token and refresh token doesn't exist | ||
if (revokedToken === 'access' && accessToken && accessToken.tokenType === 'DPoP' && !refreshToken) { |
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.
Also, theoretically, the token type is dictated by the .../token
endpoint's token_type
value. So when populating the authorization header, it should use
Authorization: ${token.token_type} ${token.access_token}
Other types of tokens returned from the server could indicate other token types, but according to the RFC the value must be DPoP. So I think this is safe here.
lib/oidc/getUserInfo.ts
Outdated
const keyPair = await findKeyPair(accessTokenObject.dpopPairId); | ||
const proof = await generateDPoPProof({ ...options, keyPair }); | ||
options.headers = { |
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.
Agreed, is there any reason why the util can't be reused here?
d84d994
to
ded5563
Compare
@@ -344,5 +344,7 @@ describe('HTTP Requestor', () => { | |||
expect(err.meta.acr_values).toEqual('urn:okta:loa:2fa:any:ifpossible'); | |||
}); | |||
}); | |||
|
|||
// TODO: OAuthError includes response object |
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.
@jaredperreault-okta does this test need to be written?
|
||
import { exchangeCodeForTokens, getOAuthUrls } from '../../../lib/oidc'; | ||
// import { generateKeyPair } from '../../../lib/oidc/dpop'; |
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.
Is this commented-out line still necessary?
@@ -10,6 +10,8 @@ | |||
* See the License for the specific language governing permissions and limitations under the License. | |||
*/ | |||
|
|||
// TODO: mock findKeyPair rather than load indexeddb? |
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.
TODO?
// Error thrown after an unsuccessful network request which requires an Authorization header | ||
// and returns a 4XX error with a www-authenticate header. The header value is parsed to construct | ||
// an error instance, which contains key/value pairs parsed out | ||
export default class WWWAuthError extends CustomError { |
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.
Cool! I wonder if there's a way to leverage this utility for step up auth error responses. Will have to experiment. I ended up having to write my own WWW-Authenticate header parser
No description provided.