ING-662 cbauthx cert auth #342

Westwooo · 2025-09-30T14:11:43Z

WIP: need to resolve https://jira.issues.couchbase.com/browse/ING-1313

cbauthx/authcheck_http.go

cbauthx/cbauth.go

cbauthx/certcheck_cached.go

chvck · 2025-10-13T13:47:01Z

cbauthx/certcheck_cached.go

+		}
+	}
+
+	if slowEntry.ResolveErr != nil {


I think we can get here without unlocking too

I don't think that we can as we either unlock in the above if, or slowEntry.PendingCh is nil which means that the checkThread finished, and we unlock at the end of that.

Do we have code like this elsewhere? Relying on a different "thread" to release a lock seems like a bug in waiting. The way that the locking between the two threads is reliant on the state of the pendingCh just seems like it's going to get broken in the future.

Do we have code like this elsewhere?

Yeah I copied the logic from authcheck_cached.go

We do I copied it from authcheck_cached.go

I don't like it but fair enough

I've been thinking about it and I think there is a case where the lock never gets dropped (pls let me know if you disagree):

// Two callers A and B call checkSlow for the same cert // A acquires the slow lock, so B is blocked // There is no slow entry so A starts a checkThread and adds an entry to the slowCache // in checkSlow A slowEntry.PendingCh != nil, because the checkThread is blocked by checkSlowA holding the lock // checkSlow A drops the lock and waits on pendingCH // Now either checkThread A or checkSlow B will acquire the lock // If checkThread A acquires the lock it rebuilds the cache, sets the slowEntry.pendingCh to nil and closes the pendingCh // checkSlow B now acquires the lock and there is a slow cache entry so no new check thread is started // slowEntry.PendingCh == nil so we skip the IF and don't drop the lock // slowCheck B completes without dropping the lock

I think this can be fixed by moving the unlock to before the if, which I've done.

Yeah that does seem like a potential deadlock, should probably file a bug against authcheck_cached. The very fact this has to be thought about so much makes me think it's too complicated...

There has been a bit of confusion here I think. The original code indeed had a bug where the Unlock was incorrectly placed inside of the if-block when it shouldn't be, but it looks like Jack has moved that Unlock call back outside of the if. There is no case where there is a cross-thread unlock. Both the checkSlow function manages the lock and the checkThread thread manages the lock, each completely independently of the other.

cbauthx/certcheck_cached.go

brett19 · 2025-10-20T18:36:14Z

cbauthx/certcheck_cached.go

+		}
+	}
+
+	if slowEntry.ResolveErr != nil {


There has been a bit of confusion here I think. The original code indeed had a bug where the Unlock was incorrectly placed inside of the if-block when it shouldn't be, but it looks like Jack has moved that Unlock call back outside of the if. There is no case where there is a cross-thread unlock. Both the checkSlow function manages the lock and the checkThread thread manages the lock, each completely independently of the other.

brett19 · 2025-10-20T18:39:23Z

cbauthx/certcheck_cached.go

+	cr.fastCache.Store(fastCache)
+}
+
+func (a *CertCheckCached) CheckCertificate(ctx context.Context, connState *tls.ConnectionState) (UserInfo, error) {


Why does this accept a *tls.ConnectionState rather than a *tls.Certificate? It looks like the only part of the ConnectionState that is actually used is PeerCertificate[0], which is referenced in a couple of places. In order to prevent a case where the object used to hash the key and the actual object used end up being different, why not 'pick out' the certificate to validate at the top-level and just pass that through? Relatedly, are we use that we only need to send PeerCertificate[0]? There are certificate chain related things that might be relevant here I think.

I've changed it so that we get the leaf at the top level and then pass that around.

Are we sure that we only need to send PeerCertificate[0].

Yeah for a couple of reasons, first because this is how they use this endpoint in their own implementation: https://github.com/couchbase/cbauth/blob/164ea251e2c977ca0a911b97e60dbdde515f6085/cbauthimpl/impl.go#L1601

Secon this endpoint does not appear to do any tls verification on the given cert. I created an unsigned certificate with CN set to an existing user with:

➜ node_certs openssl genrsa -out unsigned_client.key 2048 ➜ node_certs openssl req -x509 -new -nodes -key unsigned_client.key -sha256 -days 3650 -out unsigned_client.crt \ -subj "/C=US/ST=State/L=City/O=MyOrg/CN=Westwooo"

I manually sent this to the extractUserFromCert endpoint and got a 200 response, with the username and Uuid.

brett19 · 2025-10-20T18:39:59Z

cbauthx/certcheck_cached.go

+
+	fastCertCache := a.fastCache.Load()
+	if fastCertCache != nil {
+		userInfo, wasFound := fastCertCache.users[certHash]


nit: This object should probably be called .certificates or .certs rather than .users I think?

I think certs would be confusing since it is a map of users, have changed the name to make it more clear though, let me know your thoughts.

ING-662: Implement cert caching and add unit tests ING-662: Finish client cert auth implementation

Westwooo force-pushed the ING-662_cbauthx-cert-auth branch 3 times, most recently from 354d590 to 54f79f0 Compare September 30, 2025 14:29

chvck requested changes Oct 13, 2025

View reviewed changes

Westwooo force-pushed the ING-662_cbauthx-cert-auth branch 4 times, most recently from 97ec8f7 to b94ae87 Compare October 16, 2025 10:01

Westwooo requested a review from chvck October 20, 2025 06:44

brett19 requested changes Oct 20, 2025

View reviewed changes

Westwooo force-pushed the ING-662_cbauthx-cert-auth branch 2 times, most recently from 436ecf1 to c1be669 Compare October 21, 2025 07:18

Brett Lawson and others added 4 commits October 21, 2025 13:20

ING-662: Added support for certificate authentication to cbauthx.

57783d2

ING-662: Implement cert caching and add unit tests ING-662: Finish client cert auth implementation

ING-662: Refactor common error handling in cached auth checkers

7048d0b

ING-662: Separate UserInfo and http response structs

d99410a

ING-662: Avoid edge case where lock may not be dropped

97cfa08

Westwooo force-pushed the ING-662_cbauthx-cert-auth branch 4 times, most recently from a98ad68 to d730547 Compare October 21, 2025 12:46

ING-662: Extract leaf cert from tls conn at top level

8a7c238

Westwooo force-pushed the ING-662_cbauthx-cert-auth branch from d730547 to 8a7c238 Compare October 21, 2025 12:48

brett19 approved these changes Oct 21, 2025

View reviewed changes

chvck approved these changes Oct 22, 2025

View reviewed changes

Westwooo merged commit 491da08 into master Oct 22, 2025
15 of 17 checks passed

Westwooo deleted the ING-662_cbauthx-cert-auth branch October 22, 2025 09:57

ING-662 cbauthx cert auth #342

ING-662 cbauthx cert auth #342

Uh oh!

Conversation

Westwooo commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Westwooo Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Westwooo Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Westwooo commented Sep 30, 2025 •

edited

Loading

Westwooo Oct 14, 2025 •

edited

Loading

Westwooo Oct 21, 2025 •

edited

Loading