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

Fix issue with MapClaims VerifyAudience []string #12

Merged
merged 3 commits into from
May 29, 2021
Merged

Conversation

Waterdrips
Copy link
Member

There was an issue in MapClaims's VerifyAudiance where a []string (which
is valid in the spec) would return true (claim is found, or nil) when required
was not set.
It now checks interface types correctly and has tests written

Fixes: #6
Signed-off-by: Alistair Hey alistair@heyal.co.uk

There was an issue in MapClaims's VerifyAudiance where a []string (which
is valid in the spec) would return true (claim is found, or nil) when required
was not set.
It now checks interface types correctly and has tests written

Signed-off-by: Alistair Hey <alistair@heyal.co.uk>
@Waterdrips
Copy link
Member Author

This is validating against this part of the RFC https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.3

4.1.3.  "aud" (Audience) Claim

   The "aud" (audience) claim identifies the recipients that the JWT is
   intended for.  Each principal intended to process the JWT MUST
   identify itself with a value in the audience claim.  If the principal
   processing the claim does not identify itself with a value in the
   "aud" claim when this claim is present, then the JWT MUST be
   rejected.  In the general case, the "aud" value is an array of case-
   sensitive strings, each containing a StringOrURI value.  In the
   special case when the JWT has one audience, the "aud" value MAY be a
   single case-sensitive string containing a StringOrURI value.  The
   interpretation of audience values is generally application specific.
   Use of this claim is OPTIONAL.

@oxisto
Copy link
Collaborator

oxisto commented May 28, 2021

Ah perfect, a fix without touching the claims struct 👍

claims.go Outdated Show resolved Hide resolved
oxisto added a commit that referenced this pull request May 28, 2021
Starting with Go 1.7, since this is the lowest version still working, once we merge in #12. We can then gradually move upwards.
@Waterdrips
Copy link
Member Author

Have we decided what go versions to support and if we are using github actions OR travis ?

Keep aud validation using constant time compare by not instantly
returning on a true comparison, keep comparing all options and store
result in a variable

Signed-off-by: Alistair Hey <alistair@heyal.co.uk>
@oxisto
Copy link
Collaborator

oxisto commented May 28, 2021

Have we decided what go versions to support and if we are using github actions OR travis ?

Basically I have enabled GH actions for those versions we want to support (>= 1.11). Travis has 1.7-1.10 currently. My idea is then to gradually remove versions from it (Travis), when PRs reasonably demand it, until Travis can be removed completely.

Copy link
Collaborator

@oxisto oxisto 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
Member

@lggomez lggomez left a comment

Choose a reason for hiding this comment

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

LGTM

@lggomez lggomez merged commit 0f726ea into master May 29, 2021
@lggomez lggomez deleted the waterdrips-fix-cve branch May 29, 2021 01:45
yarlson added a commit to yarlson/go-jira that referenced this pull request Jul 19, 2021
andygrunwald#343

 https://github.com/dgrijalva/jwt-go has been abondoned (see dgrijalva/jwt-go#462). In order to fix the vulnarability we have to switch to a community driven fork https://github.com/golang-jwt/jwt . The issue has been fixed in golang-jwt/jwt#12
yarlson added a commit to yarlson/go-jira that referenced this pull request Jul 19, 2021
andygrunwald#343

 https://github.com/dgrijalva/jwt-go has been abondoned (see dgrijalva/jwt-go#462). In order to fix the vulnarability we have to switch to a community driven fork https://github.com/golang-jwt/jwt . The issue has been fixed in golang-jwt/jwt#12
lwsanty pushed a commit to lwsanty/go-jira that referenced this pull request Aug 18, 2021
andygrunwald#343

 https://github.com/dgrijalva/jwt-go has been abondoned (see dgrijalva/jwt-go#462). In order to fix the vulnarability we have to switch to a community driven fork https://github.com/golang-jwt/jwt . The issue has been fixed in golang-jwt/jwt#12

(cherry picked from commit fff481a)
@lwsanty lwsanty mentioned this pull request Aug 18, 2021
9 tasks
lwsanty added a commit to lwsanty/go-jira that referenced this pull request Aug 18, 2021
andygrunwald#343

 https://github.com/dgrijalva/jwt-go has been abondoned (see dgrijalva/jwt-go#462). In order to fix the vulnarability we have to switch to a community driven fork https://github.com/golang-jwt/jwt . The issue has been fixed in golang-jwt/jwt#12

(cherry picked from commit fff481a)

Co-authored-by: Yar Krvtsov <yarlson@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix security vulnerability (CVE-2020-26160)
3 participants