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

feat: customizable token prefix #733

Closed
wants to merge 1 commit into from

Conversation

mattslocum
Copy link

@mattslocum mattslocum commented Jan 4, 2023

matching Hydra PR: ory/hydra#3407

#675 added ory_at|pt|ac prefixes to HMAC tokens. This is a good feature for detecting tokens in places they shouldn't exist. However, for security minded self-hosted implementations, it is often preferred to avoid underlying tooling providers. In this case, it is preferrable to be able to customize the ory name and replace it with another key.

This PR adds ability to customize the token prefix before at|pt|ac.

Related Issue or Design Document

Related to #675 and ory/hydra#2845.

Checklist

  • I have read the contributing guidelines and signed the CLA.
  • I have referenced an issue containing the design document if my change introduces a new feature.
  • I have read the security policy.
  • I confirm that this pull request does not address a security vulnerability.
    If this pull request addresses a security vulnerability,
    I confirm that I got approval (please contact [email protected]) from the maintainers to push the changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added the necessary documentation within the code base (if appropriate).

Further comments

For backwards compatibility for integrations that already upgraded, it continues to remove the ory specific prefix when trimming.

@mattslocum mattslocum requested a review from aeneasr as a code owner January 4, 2023 23:44
@james-d-elliott
Copy link
Contributor

Wouldn't this be easier via a newup function which sets the prefix field then provide the result as the core strategy?

@mattslocum
Copy link
Author

Wouldn't this be easier via a newup function which sets the prefix field then provide the result as the core strategy?

Yeah, I agree that it seems like it should be easier. I was trying to match what I perceived as the pattern I found in https://github.com/ory/fosite/blob/master/config.go and https://github.com/ory/fosite/blob/master/config_default.go, and then I chased a couple of those examples and tried to mimic them. Can you expound on the better way or is there a similar example?
If you are referring to the strategy_hmacsha.go change only, then I can see how that is odd. @aeneas made the original change that added the private field, and I was struggling to figure out the full purpose. b652335#diff-180026ce292f76d8c418fa1d6c09ef637ffd461c5772ec9441b1131642965f6f But I'm happy to refactor that field to be a different style.

@james-d-elliott
Copy link
Contributor

Yeah it makes sense why you did it. I think the intention in the pattern was primarily for hot reloading, whereas this probably isn't something that would benefit much from it. We'll see what Mr Hackerman thinks and if he has a preference.

@aeneasr
Copy link
Member

aeneasr commented Jan 5, 2023

Hi, thank you for the PR and inititative!

Unfortunately, we don't plan on changing this. The point of token prefixes is to allow security scanners to easily scan code for leaked credentials. If everyone does their own token prefix, it makes the whole software ecosystem more vulnerable. Any token leaks will stay undetected if every site decides to use their own token prefixes. Currently, there is no standard for the token prefix format agreed upon (e.g. in an RFC). What I'm open to consider is adding a suffix to the prefix to allow big sites (assuming e.g. github wants to use this) to uniquely identify the token source, which would make it easier for scanners to tell you the token source ory_at_github_...

@aeneasr
Copy link
Member

aeneasr commented Jan 5, 2023

Whoops, pressed enter a bit too soon :) Also wanted to say thank you for the PR and contribution! It always sucks when a feature is not accepted upstream, but I hope the explanation makes sense!

@vivshankar
Copy link
Contributor

@aeneasr I know you have previously stated that Fosite is only meant to be used with Hydra but that wasn't my understanding when we adopted it for our own implementation. I believe there are others in the same boat.

Would you accept a change where the prefix can be set globally - maybe just a change from setPrefix to SetPrefix to let it be set over a public function? It doesn't make sense for our tokens to have a "ory_" prefix.

The alternative is for us to effectively clone the HMACStrategy and write our own without the prefix and that was my plan until this PR was submitted.

@mattslocum
Copy link
Author

@aeneasr Thank you for responding. A few thoughts

  1. "If everyone does their own token prefix, it makes the whole software ecosystem more vulnerable." I know you pointed out the absence of an RFC standard on prefixes. But if Google, Azure, Okta, JumpCloud, Ory, Github are all doing their own prefix isn't this the same thing and there is already a scan-ability problem? There may be hundreds or thousands of oauth vendors already. So how does putting ory_ in front help those other providers since they will be doing a different prefix?
  2. I'm in the self hosted space with Hydra, so we consider our tokens to be our concern with our own scanners. I wouldn't expect this prefix to be customizable on your enterprise hosted offering, but it would be nice on the Apache2.0 licensing side.
  3. We also have a concern about sharing underlying tech in an event of a CVE or any extra info for attack vectors. We try to prevent disclosure of internal tooling and our use of Hydra/Fosite is behind the scenes. Adding ory_ to our tokens might limit our ability to upgrade to Hydra2 because our Security team might have issues with it.

I understand that I might not change your mind on this. Thank you for your time either way.

@aeneasr aeneasr mentioned this pull request Jul 17, 2023
6 tasks
@K3das
Copy link

K3das commented Dec 17, 2023

Though I don't think it's feasible to try to hide your usage of hydra, I do agree with @mattslocum, especially considering that services reporting secret leaks will only report to ory, which isn't at all helpful in the case of a self-hosted hydra, where you'd want those services reporting to you...

@mitar
Copy link
Contributor

mitar commented Feb 15, 2024

I also opened #789 for this. Missed this PR. I am also +1 for having this be configurable. There are also +9 upvotes on this PR. @aeneasr please reconsider this.

@OffensiveBias-08-145
Copy link

@aeneasr

@K3das and @mattslocum both make valid points.

This has been a point of contention within our organization in order to adopt Ory fully.
Limiting the breadth of information providing potential attack vectors is just good security posture.

It is not clear to me how having different prefixes makes the software ecosystem more vulnerable? Organizations with good security posture are already doing their own internal scans for secret leaks.

From an attackers standpoint, the ory specific prefix is a gift. I now know that I do not need to waste time determining which Auth/Authz service is being used since information is handed to me in a pretty bow.

At a minimum an optional config should be available. This would ease the concerns from Security Teams and allow further adoption. If not, I foresee this will be a limiting factor for orgs to adopt Ory.

@mitar
Copy link
Contributor

mitar commented Sep 15, 2024

I think this has been done with #816. (It is not customizable, but at least it is easy to wrap it yourself with prefix you want.)

@aeneasr aeneasr closed this Sep 16, 2024
@mslocumjc
Copy link

If this was closed in response to Fosite's #816, then I don't think that is accurate since Hydra is still not customizable. I still think this would be a valuable feature for independently hosted instances to supply a unique prefix so that way ory owned tokens aren't confused with tokens minted through a self-hosted system.

@mitar
Copy link
Contributor

mitar commented Oct 8, 2024

For Hydra you should open an issue in Hydra repository.

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

Successfully merging this pull request may close these issues.

8 participants