Skip to content
This repository has been archived by the owner on Mar 22, 2024. It is now read-only.

Fix jwtIssuer to allow for Uris including scheme #425

Merged
merged 1 commit into from
Aug 9, 2023

Conversation

drewwells
Copy link
Contributor

@drewwells drewwells commented Aug 4, 2023

@kfox1111
Copy link
Contributor

kfox1111 commented Aug 4, 2023

This doesn't fix it, just breaks it in a different way.... (see production test failure) The issue I think is the jwtIssuer can have a protocol, but the list here can't? I think we just need to strip off the protocol if it exists?

@drewwells
Copy link
Contributor Author

This doesn't fix it, just breaks it in a different way.... (see production test failure) The issue I think is the jwtIssuer can have a protocol, but the list here can't? I think we just need to strip off the protocol if it exists?

jwtIssuer in the test isn't a real URL, so it's not being consumed. It's just calling localhost from what I can tell c1b1dd3#diff-c49e9f252dd5404974cdbe903c459ac8d149d730526f210eab4a52fa350de636R36

@drewwells
Copy link
Contributor Author

I can't get this chart to install on kind... so it's hard to test these locally. I think something is off with how the production test is working. I did install this version of the chart with our production config successfully.

@kfox1111
Copy link
Contributor

kfox1111 commented Aug 4, 2023

The idea was to be able to specify the setting once. not once for the issuer and a second time in the domain list.

It is set by default in the chart here:
https://github.com/spiffe/helm-charts/blob/main/charts/spire/values.yaml#L12

so the domains list is the set of the jwtIssuer and the additional domains and this is breaking that.

So, I think the right thing to do is still to strip off the protocol off the jwtIssuer if it exists when used in the domain list, so we can set it just once.

@kfox1111
Copy link
Contributor

kfox1111 commented Aug 4, 2023

I can't get this chart to install on kind... so it's hard to test these locally. I think something is off with how the production test is working. I did install this version of the chart with our production config successfully.

if you bring up a fresh kind cluster, then from the root of the repo, run:
./examples/production/run-tests.sh

Does that work? (Maybe thats what your doing?)

I don't use kind, but minikube, and it does work for me in that environment. But there shouldn't be much difference between the true.

@drewwells
Copy link
Contributor Author

drewwells commented Aug 5, 2023

The rendering has a bug with empty array, fixing it.

@faisal-memon faisal-memon added this to the 0.12.0 milestone Aug 8, 2023
@faisal-memon faisal-memon added the bug Something isn't working label Aug 8, 2023
    use a real URI for the production example as a test case for this

Signed-off-by: Drew Wells <dwells@infoblox.com>
Copy link
Contributor

@kfox1111 kfox1111 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@marcofranssen marcofranssen left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@marcofranssen marcofranssen changed the title domains can not include URIs fixes #424 Fix jwtIssuer to allow for Uris including scheme Aug 9, 2023
@marcofranssen marcofranssen merged commit bfec27e into spiffe:main Aug 9, 2023
19 checks passed
marcofranssen added a commit that referenced this pull request Aug 18, 2023
* 5e2e8a9 Adds AWS KMS KeyManager support (#435)
* 77fe43f Cron job to check for and update images (#249)
* b7e1525 Allow job hooks to be disabled (#434)
* 5e4cf6f Clarify project issues identified with nesting document (#450)
* 7289351 Update spire bits to 1.7.2 (#452)
* dc8a454 Array spacing in values is incorrect in a file. (#451)
* 94326d9 Fixup Helm docs
* ae8941c Support Nested Spire with External Agent (#117)
* f40743d Improve Tornjak documentation (#439)
* 0124f63 Bypass example-test for docs only changes (#449)
* 48a2898 Fix chainguard image references as per issue 442 (#443)
* bd393e9 Bump test chart dependencies (#445)
* a52818a Add a FAQ and switch rare issue from README to it (#437)
* e60f528 option to set KeyManager memory in spire server (#444)
* a167ce6 Bump actions/setup-go from 4.0.1 to 4.1.0
* e774584 Bump test chart dependencies (#426)
* bfec27e Fix jwtIssuer to allow for Uris including scheme (#425)
* 7a6e4f8 Change Tornjak backend default port (#436)
* 1e3039c Bump spire Helm Chart version from 0.11.0 to 0.11.1 (#419)
* d2e1606 issuer naming should respect issuer_name override (#378)
* a2e5c36 Bump test chart dependencies (#416)
* a09e054 support annotations so oidc can be annotated (#391)
* 7d94b10 Update spire to 1.7.1 (#412)
* 9f4d4ac Add aws_pca to the spire-server (#404)
* af13f1f Bump test chart dependencies (#401)
* 9a6768b Add support for disabling container selectors (#399)
* 4687e20 Merge pull request #315 from spiffe/persistence-type
* e16210c Merge branch 'main' into persistence-type
* 624ca9c Remove misadded lockfile (#400)
* 7ce67c6 Bump actions/checkout from 3.5.2 to 3.5.3 (#395)
* b85ba64 Bump helm/kind-action from 1.7.0 to 1.8.0 (#396)
* a6bdb4d Add persistence type flag

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
faisal-memon pushed a commit that referenced this pull request Aug 21, 2023
Please review the below changelog to ensure this matches up with the
semantic version being applied.

> **Note**: **Maintainers** ensure to run following after merging this
PR to trigger the release workflow:
>
> ```shell
> git checkout main
> git pull
> git checkout release
> git pull
> git merge main
> git push
> ```

**Changes in this release**

* 5e2e8a9 Adds AWS KMS KeyManager support (#435)
* 77fe43f Cron job to check for and update images (#249)
* b7e1525 Allow job hooks to be disabled (#434)
* 5e4cf6f Clarify project issues identified with nesting document
(#450)
* 7289351 Update spire bits to 1.7.2 (#452)
* dc8a454 Array spacing in values is incorrect in a file. (#451)
* 94326d9 Fixup Helm docs
* ae8941c Support Nested Spire with External Agent (#117)
* f40743d Improve Tornjak documentation (#439)
* 0124f63 Bypass example-test for docs only changes (#449)
* 48a2898 Fix chainguard image references as per issue 442 (#443)
* bd393e9 Bump test chart dependencies (#445)
* a52818a Add a FAQ and switch rare issue from README to it (#437)
* e60f528 option to set KeyManager memory in spire server (#444)
* a167ce6 Bump actions/setup-go from 4.0.1 to 4.1.0
* e774584 Bump test chart dependencies (#426)
* bfec27e Fix jwtIssuer to allow for Uris including scheme (#425)
* 7a6e4f8 Change Tornjak backend default port (#436)
* 1e3039c Bump spire Helm Chart version from 0.11.0 to 0.11.1 (#419)
* d2e1606 issuer naming should respect issuer_name override (#378)
* a2e5c36 Bump test chart dependencies (#416)
* a09e054 support annotations so oidc can be annotated (#391)
* 7d94b10 Update spire to 1.7.1 (#412)
* 9f4d4ac Add aws_pca to the spire-server (#404)
* af13f1f Bump test chart dependencies (#401)
* 9a6768b Add support for disabling container selectors (#399)
* 4687e20 Merge pull request #315 from spiffe/persistence-type
* e16210c Merge branch 'main' into persistence-type
* 624ca9c Remove misadded lockfile (#400)
* 7ce67c6 Bump actions/checkout from 3.5.2 to 3.5.3 (#395)
* b85ba64 Bump helm/kind-action from 1.7.0 to 1.8.0 (#396)
* a6bdb4d Add persistence type flag

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

oidc is invalid when jwtIssuer includes protocol
4 participants