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

Make WWW-Authenticate header resource parameter configurable #1150

Closed
wants to merge 1 commit into from

Conversation

craig0990
Copy link

@craig0990 craig0990 commented Sep 21, 2024

Issue: #1149

Description

Adds an additional LOWKEY_AUTH_RESOURCE argument which is used in the WWW-Authenticate header for the resource parameter. Defaults to "localhost".

Entry point

Probably with the #1149 issue.

Checklists

  • I have rebased my branch
  • My commit message is meaningful
  • The commit messages use semantic versioning: {major}, {minor} or {patch}
    • No, missed this part when reviewing existing commits - also not sure which would be best for this potential patch
  • The changes are focusing on the issue at hand
  • I have maintained or increased test coverage

Notes

Same notes as the linked issue:

  • Haven't kept the existing behaviour of using the request URI as the value of the resource parameter by default, but could presumably be amended with a conditional in the constructor. Just not 100% how Java+Spring handles unset @Value arguments (Null? Empty String?)
  • Personally not sure if using @Value in the auth filter constructor is a "good thing", but took inspiration from the VaultImporterProperties class and ... it does seem the shortest route to a possible fix
  • (Force pushed to fix the DCO/Signed-off-by footer)

One last thought:

  • Maybe this is overcomplicating things. If disableChallengeResourceVerification is in use, I guess the resource parameter doesn't matter. If it's not, it's running a custom hostname, and presumably that would be a subdomain of vault.azure.net so ... maybe it could just be straight up hard-coded in a similar way to the Basis-Theory emulator? 🤷🏻‍♂️

Signed-off-by: Craig Roberts <github@craig.craig0990.co.uk>
@nagyesta
Copy link
Owner

Closing without merge as #1151 contains these changes as well.

@nagyesta nagyesta closed this Sep 21, 2024
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.

2 participants