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

X25519 Jose #29

Merged
merged 9 commits into from
Dec 29, 2021
Merged

X25519 Jose #29

merged 9 commits into from
Dec 29, 2021

Conversation

maraino
Copy link
Contributor

@maraino maraino commented Dec 29, 2021

Description

This PR adds partial support for X25519 keys in the jose package. Enough for being able to sign and verify tokens.

In step and step-ca the support for XEdDSA is limited to a nebula provisioner, so this PR does not add support for generating JWKs with X25519 keys or similar stuff.

README.md Outdated
@@ -56,3 +56,8 @@ Package `tlsutil` provides utilities to configure tls client and servers.

Package `jose` is a wrapper for `gopkg.in/square/go-jose.v2` and implements
utilities to parse and generate JWT, JWK and JWKSets.

### x25510
Copy link
Member

Choose a reason for hiding this comment

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

x25519

jose/types.go Outdated
Comment on lines 156 to 157
EdDSA = "EdDSA" // Ed25591
XEdDSA = "XEdDSA" // Ed25591
Copy link
Member

Choose a reason for hiding this comment

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

Ed25519; XEd25519

jose/x25519.go Outdated
"go.step.sm/crypto/x25519"
)

const x25519ThumbprintTemplate = `{"crv":"X25519","kty":"OKP","x":"%s"}`
Copy link
Member

Choose a reason for hiding this comment

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

"%s" can be replaced with %q

This particular style is something that is enforced by the linters in other repo's, IIRC. This package should probably be configured to use the same linting configuration as the others.

Created #30 to track this. I can have a look at it.

jose/x25519.go Outdated
}

// SignPayload signs a payload with the current signing key using the given
// algorithm, it will fails if it's not XEdDSA.
Copy link
Member

Choose a reason for hiding this comment

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

algorithm. It will fail if it's not XEdDSA.

}

// X25519Verifier implements the jose.OpaqueVerifier interface using an X25519
// key and XEdDSA as a signing algorithm.
Copy link
Member

Choose a reason for hiding this comment

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

as the signing algorithm; reads a bit nicer.

Comment on lines +87 to +103
SigningKey{
Algorithm: XEdDSA,
Key: x25519.PrivateKey{
0x4e, 0x6c, 0xd9, 0xe8, 0x40, 0x4d, 0xe1, 0xe0,
0xc5, 0x22, 0x72, 0x8f, 0x78, 0xde, 0x88, 0x26,
0x58, 0x4b, 0x44, 0xef, 0xbc, 0xf6, 0xac, 0x10,
0x97, 0x67, 0x60, 0xcc, 0x41, 0xf1, 0x82, 0x0b,
},
},
new(SignerOptions).WithType("JWT"),
[]byte(`{"iss":"test-iss","sub":"test-sub","aud":"test-aud","exp":1234,"nbf":1200,"iat":1000,"jti":"test-jti"}`),
x25519.PublicKey{
0x51, 0xf3, 0xfe, 0x72, 0xb4, 0x35, 0x66, 0x52,
0x16, 0x16, 0x38, 0xfb, 0x9b, 0xf7, 0x17, 0x06,
0x87, 0xb6, 0x35, 0x53, 0xc1, 0x63, 0x9c, 0x73,
0xa7, 0x79, 0x1e, 0xd2, 0xba, 0xf8, 0xcb, 0x67,
},
Copy link
Member

@hslatman hslatman Dec 29, 2021

Choose a reason for hiding this comment

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

Is there a reason you're not using priv and pub for this test case (and the next one)? Are these known x25519 test keys?

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 prefer sometimes to use fixed values, as they might reveal problems in other areas.

Comment on lines +244 to +248
buf := mustTeeReader(t)
rand.Reader = buf

// Random
buf.Write([]byte{
Copy link
Member

Choose a reason for hiding this comment

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

It took me a little while for me to understand that you're effectively preparing deterministic output from rand.Reader. Might be good to add a comment to indicate that that's what's happening.

I'm curious to know if setting rand.Reader in this test case has consequences for other tests running concurrently. I'm aware that mustTeeReader should reset the Reader when the test finishes, but multiple tests might still be reading from this custom random at the same time, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should only be a problem if we run the tests in parallel using t.Parallel(). I'll add a comment to that mustTeeReader method, but we're using this in a lot of places. For example, this is a way to do unit tests on any non-deterministic signature schema like ECDSA. XEdDSA vectors use the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, mustTeeReader is not necessary, as we can only really need a buffer. But I've been using it by habit.

}

func TestX25519Signer_Public(t *testing.T) {
pub, priv, err := x25519.GenerateKey(rand.Reader)
Copy link
Member

Choose a reason for hiding this comment

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

While investigating the deterministic output for rand.Reader, I saw that the Go stdlib implementation of ed25519.GenerateKey checks if rand.Reader is nil, and if so, will fallback to crypto/rand.Reader. It might be nice to mirror that approach for x25519.GenerateKey, unless you expect problems down the road?

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 prefer not to do that, as it can hide other errors, it will probably panic if it's nil.

jose/types.go Outdated
Comment on lines 156 to 157
EdDSA = "EdDSA" // Ed25591
XEdDSA = "XEdDSA" // X25591 with XEdDSA signature schema
Copy link
Member

Choose a reason for hiding this comment

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

Ed25519 and X25519

@maraino maraino merged commit 2dc253d into master Dec 29, 2021
@maraino maraino deleted the x25519-jose branch December 29, 2021 19:36
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.

None yet

2 participants