Skip to content

Support for unsigned token - #54

Merged
coheigea merged 2 commits into
apache:masterfrom
amergey:unsigned-tokens
Jun 17, 2020
Merged

Support for unsigned token#54
coheigea merged 2 commits into
apache:masterfrom
amergey:unsigned-tokens

Conversation

@amergey

@amergey amergey commented Jun 12, 2020

Copy link
Copy Markdown
Contributor

No description provided.

@coheigea coheigea left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that "doNotEnforceAssertionsSigned" should only apply to encrypted assertions. I don't see a use-case for supporting unsigned and unencrypted assertions.

@amergey

amergey commented Jun 16, 2020

Copy link
Copy Markdown
Contributor Author

I think that "doNotEnforceAssertionsSigned" should only apply to encrypted assertions. I don't see a use-case for supporting unsigned and unencrypted assertions.

Then maybe the doNotEnforceAssertionSigned in configuration is not necessary at all and replace its "logic" with checks to the token. Assertion signature check whould be disabled when it is crypted and unsigned. What do you think ?

@coheigea

Copy link
Copy Markdown
Contributor

No, I still think we should always default to requiring a signed token, even if it's encrypted.

@amergey
amergey requested a review from coheigea June 16, 2020 14:43

@coheigea coheigea left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A minor change which I'll do myself is that it should check the signature in all cases (even if not required).

@coheigea
coheigea merged commit 7f9aa1c into apache:master Jun 17, 2020
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