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

Add the ability to disable DPoP #566

Closed
mkleene opened this issue Apr 11, 2024 · 7 comments
Closed

Add the ability to disable DPoP #566

mkleene opened this issue Apr 11, 2024 · 7 comments
Assignees
Labels
adr Architecture Decision Records pertaining to OpenTDF

Comments

@mkleene
Copy link
Contributor

mkleene commented Apr 11, 2024

Customers will have a variety of IdPs. Some of them do not implement DPoP. Allowing these customers to use these IdPs could be supported by adding a switch that disables DPoP checking.

This does weaken security somewhat so it's important to consider the implications.

Another idea would be to allow the IdP to control this. In this case we would only validate tokens that we detect are DPoP via the method here: #409

@mkleene mkleene added the adr Architecture Decision Records pertaining to OpenTDF label Apr 11, 2024
@pflynn-virtru
Copy link
Member

pflynn-virtru commented Apr 11, 2024

For maximum OIDC compatibility, It would be nice for the SDK and Service be able to adjust automatically to the capabilities of the IdP.

  • SDK sends dpop header to token_endpoint, if it does not get a cnf field in the JWT, then set Bearer in Authorization header.
  • Service authentication does standard checks if Bearer is set, else DPoP check
  • Add configuration to set server.auth.dpop.required=true if the administrator of the OpenTDF service requires it.

@mkleene
Copy link
Contributor Author

mkleene commented Apr 11, 2024

For maximum OIDC compatibility, It would be nice for the SDK and Service be able to adjust automatically to the capabilities of the IdP.

* SDK sends `dpop` header to token_endpoint, if it does not get a `cnf` field in the JWT, then set `Bearer` in `Authorization` header.

* Service  authentication does standard checks if `Bearer` is set, else DPoP check

* Add configuration to set `server.auth.dpop.required=true` if the administrator of the OpenTDF service requires it.

I think that the second point isn't the best way to do things since it would enable bypassing DPoP. We should do things based on the structure of the token we are passed as the default, I think.

I like adding a config option that makes us enforce DPoP!

@damorris25
Copy link
Member

Agreed - we should allow the administrator to configure if DPoP is enabled or not. Trying to make the SDK's 'smart' enough to figure out if DPoP is enabled is likely a way to open a door for attack vs simply making it an administrator switch. That would match with how many other security settings are controlled in the overall software stack.

@strantalis
Copy link
Member

@dmihalcik-virtru We plan on moving forward with adding a enforce dpop setting in the platform so by default it will be more of a soft check. If dpop is presented in the request we will validate it. Otherwise we will just validate the bearer token. This means KAS will ignore checking the signed request body.

Can you confirm we don't lose anything by not checking that signed body in a rewrap request?

@mkleene
Copy link
Contributor Author

mkleene commented Apr 17, 2024

@strantalis we definitely do lose something by not checking the body signature. For example rewrap; if an eavesdropper gets ahold of an auth token and the associated DPoP header (which asserts verb + path) they can unwrap any TDF they want, since the verb and path will be the same.

If we have a body signature then an eavesdropper can just rewrap the single TDF that they intercepted the request for.

Maybe putting stable, unique, identifier of the TDF in the path could mitigate this? Probably doesn't work for GRPC, though.

@mkleene
Copy link
Contributor Author

mkleene commented Apr 17, 2024

@dmihalcik-virtru We plan on moving forward with adding a enforce dpop setting in the platform so by default it will be more of a soft check. If dpop is presented in the request we will validate it. Otherwise we will just validate the bearer token. This means KAS will ignore checking the signed request body.

Can you confirm we don't lose anything by not checking that signed body in a rewrap request?

This is probably splitting hairs but I'd say that by default it will be a hard check. Administrators that have IdPs that don't do what we need them to can disable the check but by default we will be enforcing DPoP.

@mkleene mkleene self-assigned this Apr 17, 2024
github-merge-queue bot pushed a commit that referenced this issue Apr 23, 2024
* add `auth.enforceDPoP`. enabling this setting tells the auth
middleware to accept tokens that do not have a `cnf` claim as valid.
since having DPoP or not is a property this config goes along with each
issuer
* change the return of `checkToken` to return a context that has the
right stuff in it so that we don't have to validate that something that
came back without an error is non-nil
* make it possible to disable auth on rewrap. currently we can't because
we require there be a dpop JWK

addresses #566

---------

Co-authored-by: Dave Mihalcik <dmihalcik@virtru.com>
@mkleene
Copy link
Contributor Author

mkleene commented Apr 23, 2024

addressed by: #617

@mkleene mkleene closed this as completed Apr 23, 2024
tech-guru42 added a commit to tech-guru42/TDF that referenced this issue Jun 3, 2024
* add `auth.enforceDPoP`. enabling this setting tells the auth
middleware to accept tokens that do not have a `cnf` claim as valid.
since having DPoP or not is a property this config goes along with each
issuer
* change the return of `checkToken` to return a context that has the
right stuff in it so that we don't have to validate that something that
came back without an error is non-nil
* make it possible to disable auth on rewrap. currently we can't because
we require there be a dpop JWK

addresses opentdf/platform#566

---------

Co-authored-by: Dave Mihalcik <dmihalcik@virtru.com>
passion-127 added a commit to passion-127/TDF that referenced this issue Jun 6, 2024
* add `auth.enforceDPoP`. enabling this setting tells the auth
middleware to accept tokens that do not have a `cnf` claim as valid.
since having DPoP or not is a property this config goes along with each
issuer
* change the return of `checkToken` to return a context that has the
right stuff in it so that we don't have to validate that something that
came back without an error is non-nil
* make it possible to disable auth on rewrap. currently we can't because
we require there be a dpop JWK

addresses opentdf/platform#566

---------

Co-authored-by: Dave Mihalcik <dmihalcik@virtru.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adr Architecture Decision Records pertaining to OpenTDF
Projects
None yet
Development

No branches or pull requests

4 participants