Alb jwt verifier #190
Alb jwt verifier #190
Conversation
|
🎉 great work @NicolasViaud ! And super important. Will review incrementally, likely over the next few days. For now, can you run |
|
And maybe to be sure |
src/alb-verifier.ts
Outdated
| const regions = albArn.map(extractRegion); | ||
| const uniqueRegions = Array.from(new Set(regions)); | ||
| if (uniqueRegions.length > 1) { | ||
| throw new ParameterValidationError("Multiple regions found in ALB ARNs"); |
There was a problem hiding this comment.
Good catch hadn’t considered this yet! Side effect of allowing multiple ALB ARNs in one configuration is that you have to guard against this now.
I think it’s not unreasonable to throw this error. It’s probably very niche to create a verifier for multiple ALBs in different regions, I can’t see why you would ever want that.
What's your take?
There was a problem hiding this comment.
I also think it's a niche usage to have a compute instance (where this lib is present), that are targeted by several ALB from different regions, even though it's apparently possible with a cross region VPC peering.
But I don't think that it's a current limitation of the implementation. The correct configuration for this use case would be to use the AlbJwtVerifier with multiple properties, and ALB ARNs from different regions shouldn't be inside the same property element. Ex:
const verifier = AlbJwtVerifier.create(
[{
albArn:
"arn:aws:elasticloadbalancing:eu-west-1:123456789012:loadbalancer/app/my-load-balancer/",
issuer:
"https://cognito-idp.us-east-1.amazonaws.com/us-east-1_def",
clientId: "client1",
},
{
albArn:
"arn:aws:elasticloadbalancing:eu-west-2:123456789012:loadbalancer/app/my-load-balancer/",
issuer:
"https://cognito-idp.us-east-2.amazonaws.com/us-east-2_def",
clientId: "client2",
}]
);It should work as far as there is 2 different issuers (but is it really possible to have only one issuer for 2 ALB in 2 region ?!)
And my thougt is that we shouldn't really overengineer it for now, because he should cover most of use cases. And if someone is in a really niche use case that we havn't thought about, we should see then how to support it
Is it ok for you @ottokruse ?
There was a problem hiding this comment.
Yes, let's accept this as an edge case.
There was a problem hiding this comment.
So TL;DR you cannot use ALBs from different regions inside the same verifier instance, unless they have different issuers.
If you really need that, you can create the generic JwtVerifier but will need to add the ALB specific safeguards (against ALBeast) then. However you can use our new ALB cache then (right?)
add unit test for alb-verifier.ts (reach 100% coverage)
add unit tests for cache.ts and alb-cache.ts
- check all the time that the alb arn is valid - check, only when jwksUri is not specified, that all alb arn associated to one issuer have the same region + Improve the error message when multiple regions
#109
Description of changes:
Proposition for an AwsAlbJwksCache implementation.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.