Skip to content

ING-1329: Add testing for client cert auth#315

Merged
Westwooo merged 1 commit into
masterfrom
ING-1329-client-cert-testing
Nov 20, 2025
Merged

ING-1329: Add testing for client cert auth#315
Westwooo merged 1 commit into
masterfrom
ING-1329-client-cert-testing

Conversation

@Westwooo
Copy link
Copy Markdown
Contributor

This PR adds support for testing client cert auth. It only tests client cert auth for kv ops at the moment, since this is the only place that has client cert auth working. I'll fix client cert auth for HTTP services and Data API in ING-1339 and I'll add tests for those services then.

@Westwooo Westwooo force-pushed the ING-1329-client-cert-testing branch from 1377d19 to ba40c86 Compare November 12, 2025 12:09
@Westwooo Westwooo changed the title ING-1329: Add support for testing client cert auth ING-1329: Add testing for client cert auth Nov 12, 2025
@Westwooo Westwooo force-pushed the ING-1329-client-cert-testing branch 3 times, most recently from 6e7a223 to 63f8a0c Compare November 14, 2025 13:52
Comment thread gateway/test/mtls_test.go
Comment thread gateway/test/mtls_test.go
@Westwooo Westwooo force-pushed the ING-1329-client-cert-testing branch from 63f8a0c to 3e2b864 Compare November 17, 2025 09:41
@Westwooo Westwooo requested review from brett19 and chvck November 17, 2025 11:43
Comment thread cmd/gateway/main.go Outdated
var selfSignedCert *tls.Certificate
if config.selfSign {
generatedCert, err := selfsignedcert.GenerateCertificate()
generatedCert, generatedKey, err := selfsignedcert.GenerateCaCertificate()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is the purpose of splitting this into two steps?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here nothing. I made GenerateCaCert return the cert and key so that we could use it to generate signed certs in other places. But that means that in places we don't use the cert and key separately we need to turn them into a tls cert.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess the piece that I was confused by was that everywhere in this PR the certificate and key are immediately passed on to ConstructTlsCert and they key is not used independently. Additionally, the fact that our "self-signed" option does not expose the key in any way was intentional as it is not meant to represent a 'valid' setup of CNG that you could actually pass certificate validation for, but rather simply a way to quickly start CNG with certificate validation disabled.

@Westwooo Westwooo requested a review from brett19 November 18, 2025 08:18
Copy link
Copy Markdown
Collaborator

@chvck chvck left a comment

Choose a reason for hiding this comment

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

+1, will wait for Brett to +2

@Westwooo Westwooo force-pushed the ING-1329-client-cert-testing branch 2 times, most recently from 5f24b5c to 0ab78fe Compare November 20, 2025 10:30
@Westwooo Westwooo force-pushed the ING-1329-client-cert-testing branch from 0ab78fe to 3bd4a3b Compare November 20, 2025 12:29
@Westwooo Westwooo merged commit 89a8ca8 into master Nov 20, 2025
26 checks passed
@Westwooo Westwooo deleted the ING-1329-client-cert-testing branch November 20, 2025 20:11
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