feat: customizable token prefix

[mattslocum]

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 security@ory.sh) 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.

[james-d-elliott]

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

[mattslocum]

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]

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]

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]

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]

@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]

@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.

[K3das]

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]

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.