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

Allow multiple JWT tokens to be configured (closes #108) #109

Merged
merged 14 commits into from
Jan 11, 2024

Conversation

keeganwitt
Copy link
Contributor

No description provided.

@faisal-memon
Copy link
Collaborator

@keeganwitt Thanks for the submission. Couple questions:

  • What does the hcl config look like with this change?
  • What do you think my suggestion of auto suffixing the audience to the file name?

@keeganwitt
Copy link
Contributor Author

keeganwitt commented Dec 14, 2023

  • What does the hcl config look like with this change?

My intention was for it to look something like this (and maybe the way I've got it in my PR isn't quite right)

"jwt_svids": [
  {
    "jwt_audience": "some-audience",
    "jwt_svid_file_name": "some/path/some-audience.jwt"
  }
]
  • What do you think my suggestion of auto suffixing the audience to the file name?

This might be an acceptable approach. In our specific case, that would be acceptable. But my thinking was what if you required these JWTs to be in different Kubernetes volumes? There'd be no way to do this. So I thought it was better to choose an option that would offer more flexibility.

@keeganwitt
Copy link
Contributor Author

I discovered #112 while working on this, which is probably a pre-requisite.

@faisal-memon
Copy link
Collaborator

  • What does the hcl config look like with this change?

My intention was for it to look something like this (and maybe the way I've got it in my PR isn't quite right)

"jwt_svids": [
  {
    "jwt_audience": "some-audience",
    "jwt_svid_file_name": "some/path/some-audience.jwt"
  }
]

Seems about right.

  • What do you think my suggestion of auto suffixing the audience to the file name?

This might be an acceptable approach. In our specific case, that would be acceptable. But my thinking was what if you required these JWTs to be in different Kubernetes volumes? There'd be no way to do this. So I thought it was better to choose an option that would offer more flexibility.

Good point.

pkg/sidecar/config.go Outdated Show resolved Hide resolved
@keeganwitt keeganwitt force-pushed the multiple-jwt-audiences branch 2 times, most recently from 298a3a1 to 7aea7b1 Compare December 19, 2023 15:39
Signed-off-by: Keegan Witt <[email protected]>
@faisal-memon
Copy link
Collaborator

@keeganwitt changes are looking good. Can you update the readme?

@faisal-memon faisal-memon added this to the 0.8.0 milestone Dec 20, 2023
Signed-off-by: Keegan Witt <[email protected]>
@keeganwitt
Copy link
Contributor Author

@keeganwitt changes are looking good. Can you update the readme?

Oops. Yes. Done.

pkg/sidecar/config.go Outdated Show resolved Hide resolved
pkg/sidecar/config.go Outdated Show resolved Hide resolved
pkg/sidecar/config.go Outdated Show resolved Hide resolved
pkg/sidecar/sidecar.go Outdated Show resolved Hide resolved
test/fixture/config/helper.conf Show resolved Hide resolved
@faisal-memon faisal-memon changed the title Allow multiple JWT audiences to be configured (closes #108) Allow multiple JWT tokens to be configured (closes #108) Dec 20, 2023
README.md Outdated Show resolved Hide resolved
Co-authored-by: Faisal Memon <[email protected]>
Signed-off-by: Keegan Witt <[email protected]>
pkg/sidecar/config.go Outdated Show resolved Hide resolved
pkg/sidecar/config.go Outdated Show resolved Hide resolved
keeganwitt and others added 3 commits December 20, 2023 17:11
Co-authored-by: Faisal Memon <[email protected]>
Signed-off-by: Keegan Witt <[email protected]>
Co-authored-by: Faisal Memon <[email protected]>
Signed-off-by: Keegan Witt <[email protected]>
README.md Outdated Show resolved Hide resolved
Signed-off-by: Keegan Witt <[email protected]>
Copy link
Collaborator

@faisal-memon faisal-memon left a comment

Choose a reason for hiding this comment

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

Thanks @keeganwitt changes look good to me. @MarcosDY is it ok with you?

@MarcosDY MarcosDY merged commit 8d4d3ab into spiffe:main Jan 11, 2024
12 checks passed
@keeganwitt keeganwitt deleted the multiple-jwt-audiences branch January 13, 2024 01:00
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.

3 participants