Skip to content
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

AWS Lambda HTTP Security Integration #17192

Merged
merged 1 commit into from
Jun 8, 2021

Conversation

patriot1burke
Copy link
Contributor

Provide some security integration for the quarkus-amazon-lambda-http and rest extensions. It extracts security metadata from the lambda http event to generate a io.quarkus.security.identity.SecurityIdentity which it attaches to the context objects in QuarkusHttpHeaders. The extension automatically adds a Vertx Web filter that will extract the SecurityIdentity and attach it to the RouteContext so that every other Quarkus HTTP framework can access it in it's standard way.

@patriot1burke
Copy link
Contributor Author

@jonathanroques @richiethom Hey guys, I expanded a lot on our security integration this week. Let me know what you think of the pull request. Here is the documentation: https://github.com/patriot1burke/quarkus/blob/aws-principal/docs/src/main/asciidoc/amazon-lambda-http.adoc

@stuartwdouglas Just want to know if its cool how I implemented this. Seems to work.

Copy link
Member

@stuartwdouglas stuartwdouglas left a comment

Choose a reason for hiding this comment

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

I think this should be changed a little bit to integrate with our existing security layer. Basically instead of sticking a SecurityIdentity into the QuarkusHttpHeaders, you stick in the raw request object, and then provide a HttpAuthenticationMechanism that will extract it and authenticate it through our normal security layer.

This will give a much more consistent user experience, as things like SecurityIdentityAugmentor will still work as expected.

@patriot1burke
Copy link
Contributor Author

@stuartwdouglas One thing I'm having a problem with mechanisms is that if there is no identity associated with the http request and my IdentityProvider returns a null identity, I get a 401. How can you make it so that if there is no identity with the request, it doesn't challenge?

@patriot1burke
Copy link
Contributor Author

@stuartwdouglas FYI for lambda security, the authentication happens at the API Gateway already and there is no authentication step Quarkus needs to do. Quarkus just needs to propagate the identity and claims passed with the http event received.

@patriot1burke
Copy link
Contributor Author

@stuartwdouglas One thing I'm having a problem with mechanisms is that if there is no identity associated with the http request and my IdentityProvider returns a null identity, I get a 401. How can you make it so that if there is no identity with the request, it doesn't challenge?

I found that if there is only one IdentityProvider that returns an optional empty identity, there is no 401 challenge. If there are two matching IdentityProviders and they both return optional empty identities, then the code automatically throws a 401.

@stuartwdouglas
Copy link
Member

That sounds like a bug, it should only challenge if authentication is required.

@patriot1burke
Copy link
Contributor Author

That sounds like a bug, it should only challenge if authentication is required.

@stuartwdouglas I made some changes to QuarkusIdentityProviderManagerImpl please see diff

If you look at the IDM code, it doesn't look like it handles null SecurityIdentity values very well. Here when the list of identity providers is exhausted, it just throws an exception

When there is only one identity provider, a different codepath is executed. Even in that case I think there are bugs. If there are augmentors, they will receive a null SecurityIdentity as a parameter.

@quarkus-bot
Copy link

quarkus-bot bot commented May 14, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building f9df78c

Status Name Step Test failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 16

@quarkus-bot
Copy link

quarkus-bot bot commented May 20, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building dd9d17c

Status Name Step Test failures Logs Raw logs
JVM Tests - JDK 11 Build Test failures Logs Raw logs
JVM Tests - JDK 11 Windows Build Test failures Logs Raw logs
JVM Tests - JDK 16 Build Test failures Logs Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 extensions/vertx-http/deployment

io.quarkus.vertx.http.security.EmptyChallengeTestCase.testNoChallenge line 42 - More details - Source on GitHub


⚙️ JVM Tests - JDK 11 Windows #

📦 extensions/vertx-http/deployment

io.quarkus.vertx.http.security.EmptyChallengeTestCase.testNoChallenge line 42 - More details - Source on GitHub


⚙️ JVM Tests - JDK 16 #

📦 extensions/vertx-http/deployment

io.quarkus.vertx.http.security.EmptyChallengeTestCase.testNoChallenge line 42 - More details - Source on GitHub

@patriot1burke patriot1burke force-pushed the aws-principal branch 2 times, most recently from 3f234a7 to 60bd78a Compare May 20, 2021 19:50
@quarkus-bot
Copy link

quarkus-bot bot commented May 20, 2021

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building 3f234a7

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented May 20, 2021

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building 60bd78a

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
JVM Tests - JDK 11 Build ⚠️ Check → Logs Raw logs
JVM Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
JVM Tests - JDK 16 Build ⚠️ Check → Logs Raw logs

@quarkusio quarkusio deleted a comment from quarkus-bot bot May 21, 2021
@patriot1burke
Copy link
Contributor Author

@stuartwdouglas This should be good now. What I particularly want review on is my changes within quarkus security. I don't know if these are bugs or features, I assumed they were bugs:

  • If multiple IDP providers are present and no provider provided a SecurityIdentity, code would challenge with 401. I changed this so it didn't.
  • Mechanisms that published multiple credential types would sometimes not be matched.

@sberyozkin
Copy link
Member

@patriot1burke

If multiple IDP providers are present and no provider provided a SecurityIdentity, code would challenge with 401. I changed this so it didn't.

Can you explain please why it should let the request go through if no Securityidentity is provided on a secured path ?

Thanks

@patriot1burke
Copy link
Contributor Author

@patriot1burke

If multiple IDP providers are present and no provider provided a SecurityIdentity, code would challenge with 401. I changed this so it didn't.

Can you explain please why it should let the request go through if no Securityidentity is provided on a secured path ?

Thanks

A 401 is thrown by the IdpManager if no IDP can resolve an identity and the challenge happens even if the Authentication Mechanism doesn't require a challenge, HttpAuthenticationMechanism.sendChallenge returns false. The Mechanism layer should decide whether a challenge is sent or not on an empty identity, not the IDP Manager layer.

BTW, There is no authentication happening with this Lambda security integration. All this integration does is propagate security identity. API Gateway does the authentication. If the endpoint doesn't require authentication, no information is passed through. This is why I thought what I originally did was much simpler, more efficient, and more appropriate.

@sberyozkin
Copy link
Member

sberyozkin commented May 25, 2021

Hi Bill @patriot1burke, sorry for a delay,

A 401 is thrown by the IdpManager if no IDP can resolve an identity and the challenge happens even if the Authentication Mechanism doesn't require a challenge, HttpAuthenticationMechanism.sendChallenge returns false. The Mechanism layer should decide whether a challenge is sent or not on an empty identity, not the IDP Manager layer.

I don't know what is the purpose of sendChallenge method - if either HttpAuthenticationMethod itself or none of the IdentityProviders can resolve the identity (agreed that perhaps the decision should be done at the auth mechanism level) then it must be 401 or whatever is appropriate, for ex, with the code flow it would be a redirect to Keycloak, etc.

As you said it was already authenticated by the Gateway, so why would you have 401 reported by Quarkus IdentityProviderManager if this PR also has its own IDP ? I see it may not find a Principal - which I guess implies that from the API Gateway's point of view it it is an anonymous request when the principal is null ? But if Quarkus endpoint itself does require an authentication then it is 401 (For example, the endpoint has @Authenticated or @RolesAllowed requirements).

(I may be missing something)

@stuartwdouglas should sendChallenge be deprecated or do you have some cases in mind where it is not needed ? (May be WebAuthn is one of those cases, etc).

There is no authentication happening with this Lambda security integration. All this integration does is propagate security identity. API Gateway does the authentication. If the endpoint doesn't require authentication, no information is passed through.
This is why I thought what I originally did was much simpler, more efficient, and more appropriate.

Sure, the Quarkus Lamda HttpAuthenticationMechanism is effectively redundant in your case but all it does it just provides a little link into Quarkus Security, in all, it is indeed likely a bit suboptimal compared to the Quarkus Lambda specific solution but I guess it is very minor and it is worth keeping it all on a common path as opposed to each extension coming up with its own security solution. I'm sure Stuart will have more convincing reasons why it is worth it.

thanks

@patriot1burke
Copy link
Contributor Author

Hi Bill @patriot1burke, sorry for a delay,

A 401 is thrown by the IdpManager if no IDP can resolve an identity and the challenge happens even if the Authentication Mechanism doesn't require a challenge, HttpAuthenticationMechanism.sendChallenge returns false. The Mechanism layer should decide whether a challenge is sent or not on an empty identity, not the IDP Manager layer.

I don't know what is the purpose of sendChallenge method - if either HttpAuthenticationMethod itself or none of the IdentityProviders can resolve the identity (agreed that perhaps the decision should be done at the auth mechanism level) then it must be 401 or whatever is appropriate, for ex, with the code flow it would be a redirect to Keycloak, etc.

As you said it was already authenticated by the Gateway, so why would you have 401 reported by Quarkus IdentityProviderManager if this PR also has its own IDP ? I see it may not find a Principal - which I guess implies that from the API Gateway's point of view it it is an anonymous request when the principal is null ? But if Quarkus endpoint itself does require an authentication then it is 401 (For example, the endpoint has @Authenticated or @RolesAllowed requirements).

(I may be missing something)

FYI, in main, if there is only one matching IDP and that IDP returns no SecurityIdentity, then there is no 401 sent. If there are two matching IDPs, then there will be a 401 sent. I just fixed it so that if there are multiple matching IDPs, don't send a 401 if none of them returned an SecurityIdentity.

Would be cool to get a review on this soon.

@patriot1burke
Copy link
Contributor Author

@stuartwdouglas @sberyozkin Bump on review/approval. Or just tell me you are busy and I'll ping devlist

@sberyozkin
Copy link
Member

@patriot1burke, Hi, sorry, I'm not subscribed to most of the notifications and check things manually...
@stuartwdouglas should be back next week

FYI, in main, if there is only one matching IDP and that IDP returns no SecurityIdentity, then there is no 401 sent. If there are two matching IDPs, then there will be a 401 sent.

Oh, this is interesting. I'd say the former condition is a bug, so we need to wait for Stuart to clarify.
One thing I believe we should avoid is allow an (anonymous ) request coming through on the request path requiring the authentication.

Thanks

@patriot1burke
Copy link
Contributor Author

@patriot1burke, Hi, sorry, I'm not subscribed to most of the notifications and check things manually...
@stuartwdouglas should be back next week

FYI, in main, if there is only one matching IDP and that IDP returns no SecurityIdentity, then there is no 401 sent. If there are two matching IDPs, then there will be a 401 sent.

Oh, this is interesting. I'd say the former condition is a bug, so we need to wait for Stuart to clarify.

I disagree. The bug is in what I fixed. For proof, look in HttpSecurityRecorder. If there is a null SecurityIdentity, then an annonymous principal is set up.

One thing I believe we should avoid is allow an (anonymous ) request coming through on the request path requiring the authentication.

Not understanding your last statement.

It should be up to the application layer on whether a non-anonymous identity is required. For instance, servlet can have secure and unsecure paths.

@sberyozkin
Copy link
Member

@patriot1burke, hi Bill,

I disagree. The bug is in what I fixed. For proof, look in HttpSecurityRecorder. If there is a null SecurityIdentity, then an annonymous principal is set up.

But is it correct ? You've spotted an inconsistency, one no-matching IDP only - no 401, more than one - 401.

If we get to the stage where a given HttpAuthenticationMechanism detects the credentials it expects it will then delegate to the registered IDPs to convert that into a Securityidentity. The situation where in a default proactive authentication case (where if the credentials are provided they must be verified), with the provided credentials, one may end up with an anonymous Securityidentity seems wrong to me - since having the anonymous identity does not accurately reflect the state of the request.

It should be up to the application layer on whether a non-anonymous identity is required. For instance, servlet can have secure and unsecure paths.

Sure - but if the credentials are provided then it should be a non-anonymous identity with the proactive authentication being on.

Note that quarkus-oidc, quarkus-smallrye-jwt have their own inlined IDPs. So if they get a token they'll never return an anonymous identity. But then what you've found with the basic auth it can be totally opposite

  • it can accept the credentials and return an anonymous identity. And possibly the same with the Gateway + Lambda IDP when given a token one can still get an anonymous identity. It would be inconsistent.

I guess that indeed one can have some situations where even with the proactive auth, and with the provided credentials, one can end up with the anonymous identity - these are likely some edge cases. IMHO it is better be more conservative - if the provided credentials can not be converted into a non-anonymous identity then something may have gone wrong

thanks

@sberyozkin
Copy link
Member

@patriot1burke

For proof, look in HttpSecurityRecorder. If there is a null SecurityIdentity, then an annonymous principal is set up.

Just one more note about it - I believe this is to support the cases where a given HttpAuthenticationMechanism does not find the credential type it expects, for example, in quarkus-oidc, both Bearer and Code mechanisms would return a null/empty identity in such cases - but if the token is there it won't let an invalid token escape with the anonymous identity, etc

@patriot1burke
Copy link
Contributor Author

@patriot1burke, hi Bill,

I disagree. The bug is in what I fixed. For proof, look in HttpSecurityRecorder. If there is a null SecurityIdentity, then an annonymous principal is set up.

But is it correct ? You've spotted an inconsistency, one no-matching IDP only - no 401, more than one - 401.

If an Mechanism is provided, it gets invoked for all JAX-RS endpoints by default. Are you saying that an identity is required if a Mechanism is present?

Maybe I'm just implementing this wrong. That the IDP Manager should not be invoked by the mechanism unless their are known credentials. I'm letting the IDP layer decide whether or not credentials are present. Maybe that's not the contract. I'll rework the PR tomorrow.

@sberyozkin
Copy link
Member

sberyozkin commented May 28, 2021

@patriot1burke Good morning,

Maybe I'm just implementing this wrong. That the IDP Manager should not be invoked by the mechanism unless their are known credentials

IMHO it makes sense - the full HttpAuthenticationMechanism -> IdentityProviderManager delegation process is not needed if CognitoPrincipal is not available.

So indeed what I'd propose to change is for the Lambda HttpAuthenticationMechanism to return a null Securityidentity immediately if no token is available while for the Lambda IdentityProivider to expect the token is there and fail with AuthenticationFailedException if it is not there - users will still be able to customize it if they really need to with their custom IdentityProvider.

Re your changes to IdentityProviderManagerImpl - it may be better to revert them for now to simplify your PR but open a dedicated issue later to deal with the problem of a single IDP producing no SecurityIdentity resulting in an anonymous identity - and 401 if it is more than one IDP - as it needs to be made consistent.

Note Stuart may suggest a different approach once he gets back early next week. But your PR is nearly ready to go in any case :-)

@gsmet
Copy link
Member

gsmet commented Jun 1, 2021

@stuartwdouglas could you have another look at this one?

@sberyozkin
Copy link
Member

I'm going to open an issue to refer to the problem Bill @patriot1burke spotted, IMHO it would be worth having some discussion there without overloading this PR's review, even if the issue I'll open will be resolved by this PR as well

@patriot1burke
Copy link
Contributor Author

@sberyozkin I reverted the IDPManager changes. @stuartwdouglas Made one fix to HttpAuthenticator so that multiple request types could be supported. There was a bug that a 2nd request type might not match.

Would be cool to get a review on this assuming that CI passes. This has been around for a few weeks now.

@stuartwdouglas
Copy link
Member

Sorry, I missed this in the deluge of notification I had when I got back from PTO.

* Will only be allocated if requestContext.authorizer.jwt.claims.cognito:username is set
* in the http event sent by API Gateway
*/
public class CognitoPrincipal implements Principal {
Copy link
Member

Choose a reason for hiding this comment

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

I don't really like all these custom principal implementations, given that we have io.quarkus.security.identity.SecurityIdentity#getAttributes() that can be used to store any additional information attached to the identity (this applies to all the different ones, not just here).

Copy link
Contributor Author

@patriot1burke patriot1burke Jun 8, 2021

Choose a reason for hiding this comment

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

I just don't agree. Things just don't map one to one to an attribute list. JWT has claims and scopes (which is a set). IAM has a bunch of different nested metadata. Also, we already have a specific type for this stuff in the event, doesn't make any sense to pull AWS specific security metadata into a untyped string keyed hashmap. All this metadata is already unmarshalled too and to just fill up a hashmap with this unmarshalled data is double the work.

* Enable security mechanisms to process lambda and AWS based security (i.e. Cognito, IAM) from
* the http event sent from API Gateway
*/
@ConfigItem(defaultValue = "false")
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this default to true? If lambda is sending a security principal I would assume it is for a reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The whole mechanism abstraction adds overhead to every request which will exist if the user isn't using AWS security. I don't know yet if users will always use AWS security, so I'd like to keep the default false.

// there is no way in CDI to currently provide a prioritized list of IdentityProvider
// So, what we do here is to try to see if anybody has registered one. If no identity, then
// fire off a request that can only be resolved by the DefaultLambdaIdentityProvider
boolean useDefault;
Copy link
Member

Choose a reason for hiding this comment

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

I need to fix this, but it is fine as a workaround for now.

* the http event sent from API Gateway
*/
@ConfigItem(defaultValue = "false")
public boolean enableSecurity;
Copy link
Member

Choose a reason for hiding this comment

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

Same question as above.

@@ -242,7 +245,8 @@ private boolean isBinary(String contentType) {
private Principal getPrincipal(APIGatewayV2HTTPEvent request) {
Copy link
Member

Choose a reason for hiding this comment

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

I just checked locally, this method is not needed any more.

@stuartwdouglas
Copy link
Member

I have created quarkusio/quarkus-security#17 to add a priority attribute.

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 8, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 1a981b0

Status Name Step Test failures Logs Raw logs
Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs

Copy link
Member

@stuartwdouglas stuartwdouglas left a comment

Choose a reason for hiding this comment

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

Looks like there is a formatting issue, but otherwise LGTM.

sam local docs

convert lambda security to auth mechanism

default providers

add mechanism if empty credential types

use QuarkusPrincipal

fix test

don't fire mech if request not set

revert idp

remove getPrincipal

process resources
@patriot1burke patriot1burke merged commit e54aaf9 into quarkusio:main Jun 8, 2021
@sberyozkin
Copy link
Member

Good news :-)

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

Successfully merging this pull request may close these issues.

4 participants