The public key doesn't match if any field doesn't match

[ahrtr]

Fix #13381

[ahrtr]

Could anyone provide feedback on this PR? Thanks.

[serathius]

Can you add a test?

[ahrtr]

Can you add a test?

Thanks for the comment.

In theory, the current implementation isn't correct, because it regards the public keys as not matched when all fields do not match. It means that if some fields match, and others do not match, then it will regard them as matched. So I submitted this PR, it will only regard the public keys as matched when all fields match.

Actually I thought about adding unit test to cover the corner case, but eventually I gave up. Reasons are below.

Firstly, the existing unit tests in jwt_test.go already cover all the basic scenarios.

Secondly, it's difficult to create a real private & public key to hit the corner case.

Thirdly, I thought about adding some mock functions (see below), and add some fake public key & private key of json marshaled strings in jwt_test.go. When the values for "sign-method" in the –auth-token is "fake-rsa" or "fake-ecdsa", then etcd will use the mock functions. But it will have some impacts on the production environment as well and accordingly have security concern, so I gave up this approach.

type jwtOptions struct {
	SignMethod jwt.SigningMethod
	PublicKey  []byte
	PrivateKey []byte
	TTL        time.Duration

	parsePrivateKey func(key []byte) (v interface{}, err error)
	parsePublicKey func(key []byte) (v interface{}, err error)
}

What do you think?

[ahrtr]

@serathius any comments please?

[serathius]

Sorry, I don't think competent enough in Auth code to review this change. Was hoping that adding tests would show exact impact of this change.

Best to ask original author or reviewer of PR that implemented JWT #9883
cc @joelegasse @gyuho

[joelegasse]

Talk bout a blast from the past... I'm no longer active with etcd, but I'll try to add some context here.

When the code was written three years ago (2018), the Equal() methods did not exist in the crypto packages (added in Go 1.15, released in August 2020).

There is actually a subtle bug in the current logic, as described in the last line of the referenced issue. Apparently I failed to properly distribute the negation.

Current RSA stdlib implementation:

return pub.N.Cmp(xx.N) == 0 && pub.E == xx.E

vs. existing logic:

if pub != nil && pub.E != priv.E && pub.N.Cmp(priv.N) != 0 {

Since it's meant to be checking for an error, it should have been:

if pub != nil && (pub.E != priv.E || pub.N.Cmp(priv.N) != 0) {

Since E is generally a common value, this means that the current logic would accept any two keys as "matching". Not great, but without spending the rest of the day trying to remember the details around this, I can't say if the bug would be exploitable for anything other than a running system that can't verify its own tokens (or if it would just defer to the private, since that contains the public parts anyway).

Wrapping up:

From a mechanical perspective, this code does fix some incorrect logic in the key comparison that I wrote years ago. I can't say much more beyond that.

[ahrtr]

Just rebased the PR and squashed the commits.

[serathius]

Thanks @joelegasse, your comment was very helpful.
+1 based on @joelegasse comment

[ahrtr]

Rebased again although there is no conflict with the base branch.

[serathius]

cc @hexfusion @ptabor