Skip to content

[WIP] Signed JWT support #14

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

Closed
wants to merge 5 commits into from
Closed

[WIP] Signed JWT support #14

wants to merge 5 commits into from

Conversation

aykevl
Copy link
Contributor

@aykevl aykevl commented Jun 12, 2018

Work-in-progress PR for verified JWT support in irmago.

Example JWT that I get out of it:

eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJQcml2YWN5IGJ5IERlc2lnbiBGb3VuZGF0aW9uIiwiaXNzIjoidmVyaWZpY2F0aW9uX3JlcXVlc3QiLCJpYXQiOjE1Mjg4MzM5MTksInNwcmVxdWVzdCI6eyJ2YWxpZGl0eSI6MTIwLCJyZXF1ZXN0Ijp7ImNvbnRleHQiOm51bGwsIm5vbmNlIjpudWxsLCJjb250ZW50IjpbeyJsYWJlbCI6IkZhbWlseSBuYW1lIiwiYXR0cmlidXRlcyI6WyJwYmRmLnBiZGYuaWRpbi5mYW1pbHluYW1lIl19LHsibGFiZWwiOiJEYXRlIG9mIGJpcnRoIiwiYXR0cmlidXRlcyI6WyJwYmRmLnBiZGYuaWRpbi5kYXRlb2ZiaXJ0aCJdfV19fX0.qHO5wVLdbmuukKWXD1YG_cmSR6mNKV6XbZp7a8MizY5Ve6KXI6HDjkRCUbxRI39SHxzJiYhi8Ar2Lim6ofK0MQxTEVdH4Vp-Lof6VDNvpHZwBZMD1FMz1Lw8jg0Q4WJ5PMpddz9PLf-zFdZCltAOB2RQwOdxkYzrqbyM1Xb57KkVodPKmgs_TFqWG8Ki40oMD6kYUEbbrTl_3VS1cxHZpxlBOqfNWEw4Hk_Kd1qBTLsIR_lwbnA8PyFOgto4Ovjtcft2Oj5vcdXZUePsC3vU1rRCMIPuqjqOtBZ7U1nkYKxiLvfzZC5xqQMfW3avxx8W-uDId5MZ8sxNkTdvqHCKIg

jwt.go Outdated
claims := ServiceProviderJwt{
ServerJwt: ServerJwt{
ServerName: serverName,
Type: humanName,
Copy link
Member

Choose a reason for hiding this comment

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

  • Type should (in the case of disclosures) be the fixed string verification_request, see NewServiceProviderJwt()
  • You can actually reuse NewServiceProviderJwt() to build the JWT before signing it
  • We should have similar functions for signing and issuance (reusing NewSignatureRequestorJwt() and NewIdentityProviderJwt()

As to the serverName and humanName, that has now become a little messy:

  • The variable humanName should be assigned to the field ServerName, whose name is inappropriate (I think JwtIssuer would be better
  • The variable serverName should go into the JWT kid header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, somehow I managed to miss NewServiceProviderJwt(). I've now reused this function.

We should have similar functions for signing and issuance [...]

Yes, I'll add them later. I would prefer getting this one right first.

The variable serverName should go into the JWT kid header.

Does this work? At least the Java implementation appears to use iss, not kid (verified by decoding the JWT for BIG name/birthdate disclosure).

Copy link
Member

Choose a reason for hiding this comment

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

Originally we indeed used only iss; this was (1) shown to the user in the IRMA app and (2) the API server used this to lookup the verifier's public key with which to verify its JWTs. At some point we split these into two, so now:

  • The API server uses the kid aka serverName field in the header for the public key lookup
  • iss aka humanName is what is shown to the user in the IRMA app.

For example, here is the header from a decoded IRMATube JWT obtained by clicking on a movie:

{
  "typ": "JWT",
  "alg": "RS256",
  "kid": "irmatube"
}

Body:

{
  "iat": 1529269092,
  "iss": "IRMATube",
  "sub": "verification_request",
  "sprequest": { "truncated" }
}

These are made by this PHP library.

@aykevl
Copy link
Contributor Author

aykevl commented Jun 18, 2018

I've updated the PR with some bigger changes, to hopefully improve the structure of the code. An example JWT I've produced is:

eyJhbGciOiJSUzI1NiIsImtpZCI6ImR1byIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJ2ZXJpZmljYXRpb25fcmVxdWVzdCIsImlzcyI6IlByaXZhY3kgYnkgRGVzaWduIEZvdW5kYXRpb24iLCJpYXQiOjE1MjkzMjQxNTgsInNwcmVxdWVzdCI6eyJ2YWxpZGl0eSI6MTIwLCJyZXF1ZXN0Ijp7ImNvbnRleHQiOm51bGwsIm5vbmNlIjpudWxsLCJjb250ZW50IjpbeyJsYWJlbCI6IkZhbWlseSBuYW1lIiwiYXR0cmlidXRlcyI6WyJwYmRmLnBiZGYuaWRpbi5mYW1pbHluYW1lIl19LHsibGFiZWwiOiJEYXRlIG9mIGJpcnRoIiwiYXR0cmlidXRlcyI6WyJwYmRmLnBiZGYuaWRpbi5kYXRlb2ZiaXJ0aCJdfV19fX0.cf6utb805i1TsjnuDqptKtHlyboUoGDyVADeknM0AGXEaFQYiEE3DIfyORo3t4ub6d8Cl3lB7LTFCHKYyIRINzEXgw-xOSO_SD1X6W9_VE8cilwKkvXO_QH3z0v_6ighQM-uFusLIsyGQLR8mVoMPOmx4H-By-fezQ5s_EwMI2EVqZv6t8FwiUIWs36hkI6kS4u_oFK1a1xBoPAabRDxi3kRio_Y07initV9Z5CqxNcYIdtSMzhu7uq_cbygTwOM7RlyZJEk27fed11GWZGC03054UPfi3qM6tRm4wY_7MNV4oCNZvUAZZRHyJMzehtn6RarUL51gzxb1cEdtewP-w

TODO:

  • Implement this for IdentityProviderJwt (trivial)
  • maybe rename serverName to humanName in some other parts of the code? Currently *Jwt.Requestor returns HumanName but then this variable is named ServerName in the callers, which is not consistent.
  • comments

@confiks
Copy link
Contributor

confiks commented Jun 18, 2018

Could you please not force push branches, especially when they are tied to pull requests. It makes it very hard to figure out what the feedback is about. Just add new commits on top of each other. To solve merge conflicts you can merge master back in (instead of rebasing).

@aykevl
Copy link
Contributor Author

aykevl commented Jun 18, 2018

Ok, will avoid that now. Some projects actually want you to force-push so that's why I did it.

EDIT: the commit is here: 0ec9427

@aykevl
Copy link
Contributor Author

aykevl commented Jul 29, 2018

Any update on this? I need this for the new DUO issuer.

Copy link
Member

@sietseringers sietseringers left a comment

Choose a reason for hiding this comment

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

This is better now, thanks! Two things:

  • The receiver name that I commented on
  • The new dependency github.com/dgrijalva/jwt-go should be added to the Gopkg files (run dep ensure).

requests.go Outdated
@@ -427,3 +443,24 @@ func (jwt *SignatureRequestorJwt) IrmaSession() IrmaSession { return jwt.Request

// IrmaSession returns an IRMA session object.
func (jwt *IdentityProviderJwt) IrmaSession() IrmaSession { return jwt.Request.Request }

// Sign returns the signed and serialized JWT.
func (claims *ServiceProviderJwt) Sign(serverName string, sk *rsa.PrivateKey) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

The receiver of *ServiceProviderJwt struct methods is earlier called jwt, and it is not idiomatic in Go to use different receiver names for the same struct. We should probably switch to claims on every method of ServiceProviderJwt and its parent and siblings (to avoid the name clash with the jwt package).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've also updated the other two receivers.

@aykevl aykevl force-pushed the aykevl-jwt branch 3 times, most recently from 561c2d5 to 188c7da Compare July 30, 2018 14:52
@aykevl
Copy link
Contributor Author

aykevl commented Jul 30, 2018

I added the dependency, but avoided the inputs-digest line because it resulted in a merge conflict (and according to golang/dep#1224 that's probably safe).

@aykevl
Copy link
Contributor Author

aykevl commented Aug 4, 2018

Added a method to verify and parse a disjunction JWT.

@aykevl aykevl mentioned this pull request Aug 14, 2018
@aykevl
Copy link
Contributor Author

aykevl commented Aug 14, 2018

Closed in favor of #21.

@aykevl aykevl closed this Aug 14, 2018
@sietseringers sietseringers deleted the aykevl-jwt branch February 11, 2019 16:26
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