Use tls for nginx connection implementation #120

ciroque · 2023-09-29T20:14:13Z

Proposed changes

Implementation of TLS communication between NLK and NGINX Plus hosts.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

I have read the CONTRIBUTING document
If applicable, I have added tests that prove my fix is effective or that my feature works
If applicable, I have checked that any relevant tests pass after adding my changes
I have updated any relevant documentation (README.md and CHANGELOG.md)

internal/communication/factory.go

ciroque

Here it is. Large I know. Apologies for that. I have annotated most every file for reference.

internal/communication/factory.go

ciroque

More color for changed files.

client-secret.yaml

cmd/certificates-test-harness/doc.go

cmd/certificates-test-harness/main.go

cmd/configuration-test-harness/doc.go

cmd/configuration-test-harness/main.go

internal/synchronization/synchronizer.go

server-secret.yaml

.github/workflows/build-and-sign-image.yml

.github/workflows/scorecard.yml

ca-secret.yaml

internal/certification/certificates.go

internal/certification/certificates_test.go

internal/configuration/settings.go

4141done

I think this looks good as far as how the configuration is being provided and the TLS is being set up.

However I have two concerns that I think should be addressed before merging, or, if you deem it acceptable given the early stages of the project, fixed quickly afterwards.

(As we discussed offline) I think if a tls-mode is given in the ConfigMap, the library should fail to start rather than reverting to a no-tls mode of operation. The user clearly intended to enable some TLS mode and with the current operation they could think it succeeded when it did not.
Now that we are handling certificates I think we should be quite careful to make sure that any sensitive values don't wind up in logs or error reports. I saw a few debug statements where that could be the case.

I should also caveat that given the size of this PR I focused mostly on understanding the actual code rather than exhaustively going through all the documentation. I will learn to use the feature once we merge and work with you on any docs tweaks that need to happen.

deployments/rbac/clusterrole.yaml

internal/authentication/factory.go

.gitignore

cmd/nginx-loadbalancer-kubernetes/main.go

cmd/tls-config-factory-test-harness/main.go

internal/configuration/settings.go

internal/authentication/factory.go

internal/certification/certificates.go

internal/configuration/settings.go

4141done

This change along with the follow up issues look good. 👍 🐳 👍

- tweaks - Add Under Development notice - prefer References section over inline links to external documentation - address PR feedback - correct use of [!WARNING]

Bumps [github/codeql-action](https://github.com/github/codeql-action) from 2.21.4 to 2.21.5. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@a09933a...00e563e) --- updated-dependencies: - dependency-name: github/codeql-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>

Bumps [sigstore/cosign-installer](https://github.com/sigstore/cosign-installer) from 3.1.1 to 3.1.2. - [Release notes](https://github.com/sigstore/cosign-installer/releases) - [Commits](sigstore/cosign-installer@6e04d22...11086d2) --- updated-dependencies: - dependency-name: sigstore/cosign-installer dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>

- Checkpoint - DEMO - DEMO - Implements Certificates - Certificates fleshed out and tests underway - Refactoring of authorization tests required due to changes in the shape of the Certificates module. - FRT (Fix Red Tests) - minor cleanup - Base implementation of Certificate and informer - authentication factory implemented - rename, and doc start - CHECKPOINT: File creation

- Putting a bow on it - Final corrections and enhancements - Let the configuration settings determine log level

ciroque changed the title ~~Use tls for nginx connection impl~~ Use tls for nginx connection implementation Sep 29, 2023

ciroque force-pushed the use-tls-for-nginx-connection-impl branch from d7931e1 to 9f2f8f3 Compare October 23, 2023 19:48

github-advanced-security bot found potential problems Oct 23, 2023

View reviewed changes

internal/communication/factory.go Show resolved Hide resolved

ciroque force-pushed the use-tls-for-nginx-connection-impl branch from 9f2f8f3 to 52d65e8 Compare October 23, 2023 21:17

ciroque mentioned this pull request Oct 24, 2023

Add documentation for TLS configuration of NLK #115

Closed

4 tasks

ciroque force-pushed the use-tls-for-nginx-connection-impl branch from e1927ca to d8ab736 Compare October 24, 2023 18:40

This comment was marked as resolved.

Sign in to view

ciroque force-pushed the use-tls-for-nginx-connection-impl branch from d8ab736 to 3a037c6 Compare October 24, 2023 19:03

ciroque commented Oct 24, 2023

View reviewed changes

internal/communication/factory.go Show resolved Hide resolved

ciroque force-pushed the use-tls-for-nginx-connection-impl branch from 3a037c6 to 3b4913b Compare October 24, 2023 19:33

ciroque commented Oct 24, 2023

View reviewed changes

ciroque requested a review from 4141done October 24, 2023 20:38

ciroque marked this pull request as ready for review October 24, 2023 20:38

ciroque requested a review from chrisakker as a code owner October 24, 2023 20:38

ciroque commented Oct 31, 2023

View reviewed changes

internal/certification/certificates.go Outdated Show resolved Hide resolved

ciroque commented Oct 31, 2023

View reviewed changes

internal/certification/certificates_test.go Outdated Show resolved Hide resolved

ciroque commented Oct 31, 2023

View reviewed changes

internal/configuration/settings.go Show resolved Hide resolved

4141done reviewed Nov 21, 2023

View reviewed changes

ciroque mentioned this pull request Nov 27, 2023

no-tls should be an explicitly handled case #143

Closed

4141done approved these changes Nov 27, 2023

View reviewed changes

ciroque and others added 10 commits November 27, 2023 14:39

Add documentation for TLS configuration of NLK- better titles

5e1669d

- tweaks - Add Under Development notice - prefer References section over inline links to external documentation - address PR feedback - correct use of [!WARNING]

- mode to tlsMode

44ab150

- Putting the cherry on top

05dd66b

- Putting a bow on it - Final corrections and enhancements - Let the configuration settings determine log level

- Bug fix

bda3429

- remove dead code

641f0f6

Do not need an error here.

9eb4e84

Stop leaking certificates!!

6d7b421

ciroque added 3 commits November 27, 2023 14:41

Remove dead comment

ad45897

- update test for new code

183421d

- update test for new code

29148b2

ciroque force-pushed the use-tls-for-nginx-connection-impl branch from 0bdc8e6 to 29148b2 Compare November 27, 2023 22:41

Tidy go.mod

6ccc4f7

ciroque merged commit 4c03217 into main Nov 27, 2023

Use tls for nginx connection implementation #120

Use tls for nginx connection implementation #120

Uh oh!

Conversation

ciroque commented Sep 29, 2023

Proposed changes

Checklist

Uh oh!

This comment was marked as resolved.

Uh oh!

ciroque left a comment

Choose a reason for hiding this comment

Uh oh!

ciroque left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

4141done left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

4141done left a comment

Choose a reason for hiding this comment

Labels

2 participants