Serving cert file OIDC provider #4190

guilhermocc · 2023-05-30T16:40:45Z

Pull Request check list

Commit conforms to CONTRIBUTING.md?
Proper tests/regressions included?
Documentation updated?

Affected functionality
oidc discovery provider HTTPS configuration.

Description of change
Add new configuration option for oidc discovery provider, so users can now use their own certificates to be used on HTTPS connections. The new configuration structure is described below:

serving_cert_file {
	cert_file_path = "path_to_certificate_file"
	key_file_path = "path_to_private_key_file"
}

Users can now indicate the file path of the certificate and private key that are going to be loaded from the disk and used for establishing TLS connections with clients. Upon start, oidc discovery provider will start watching for file changes in the provided paths and reload the certificate used for connections.

Which issue this PR fixes
Fixes #3825

Signed-off-by: Guilherme Carvalho <[email protected]>

MarcosDY · 2023-06-13T12:44:21Z

support/oidc-discovery-provider/cert_manager_test.go

+
+	tmpDir := t.TempDir()
+
+	writeFile(t, tmpDir+keyFilePath, oidcServerKeyPem)


maybe you can use filepath.Join here, to avoid using /

MarcosDY · 2023-06-13T12:54:10Z

support/oidc-discovery-provider/cert_manager_test.go

+	certManager, err := NewDiskCertManager(&ServingCertFileConfig{
+		CertFilePath:     tmpDir + certFilePath,
+		KeyFilePath:      tmpDir + keyFilePath,
+		FileSyncInterval: 10 * time.Millisecond,


instead continuos reloading, why not to use a clock mock? and advance time when required?

This could make the test run faster, but we would still need to use require.Eventually to make some assertions since the time difference between loading a cert and moving the clock + making an assertion can cause errors.

MarcosDY · 2023-06-13T12:57:27Z

support/oidc-discovery-provider/cert_manager_test.go

+	require.NoError(t, err)
+}
+
+func assertFileDontExist(t *testing.T, filePath string, err error) {


may you this to _posix_test || _windows_test and void this if?

I did that initially, but I didn't like to have two extra files for differentiating only two string constants. Do you think it is worth it?

how about putting them inside config_XXXX_test? both are on the same package, and you will no need another file

I like the idea!

support/oidc-discovery-provider-20230612T134707Z-001.zip

MarcosDY · 2023-06-13T13:06:22Z

support/oidc-discovery-provider/main.go

+		if err != nil {
+			return nil, err
+		}
+		log.Info("Serving HTTPS using certificate loaded from disk")


may we add fields for used files?

MarcosDY · 2023-06-13T13:08:00Z

support/oidc-discovery-provider/main.go

+
+	tlsConfig := certManager.TLSConfig()
+
+	tcpListener, err := net.ListenTCP("tcp", &net.TCPAddr{Port: 8080})


is this hardcoded port ok?

It should be 443 for default HTTPS

The acme code for sure used 443. Now that we're not bound to ACME, this could probably be configurable so you don't need CAP_NET_BIND_SERVICE capabilities when testing it out...

You're right, we can provide more flexibility now, I will introduce a new addr configuration in this section.

Signed-off-by: Guilherme Carvalho <[email protected]>

azdagron · 2023-06-08T03:10:36Z

support/oidc-discovery-provider/README.md

@@ -56,13 +57,13 @@ The configuration file is **required** by the provider. It contains

 #### Considerations for Unix platforms

-[1]: One of `acme` or `listen_socket_path` must be defined.
+[1]: One of `acme`, `serving_cert_file`, `listen_socket_path` must be defined.


I think we still want the "or" on these lines (same suggestion for the others):

Suggested change

[1]: One of `acme`, `serving_cert_file`, `listen_socket_path` must be defined.

[1]: One of `acme`, `serving_cert_file`, or `listen_socket_path` must be defined.

azdagron · 2023-06-20T14:18:35Z

support/oidc-discovery-provider/main.go

+		err = server.Shutdown(context.Background())
+		if err != nil {


In general, it isn't safe to modify the err variable from the goroutine. In practice in this code it is fine because it isn't used after this point. It would be safer to use a new err variable, ideally scoped to the shutdown statement:

if err := server.Shutdown(context.Background()); err != nil { log.Error(err) }

azdagron · 2023-06-20T14:21:39Z

support/oidc-discovery-provider/main.go

 	certManager, err := NewDiskCertManager(config.ServingCertFile, log)
 	if err != nil {
 		return nil, err
 	}
+	go func() {
+		certManager.WatchFileChanges(ctx)


nit: I'll accept this code as-is since it's happening in main, but ideally the lifetime of this goroutine is controlled by the net.Listener it returns, i.e. when the listener is closed, this goroutine shuts down and the listener waits for it to close.

azdagron · 2023-06-20T14:23:42Z

support/oidc-discovery-provider/main.go

+
+	tlsConfig := certManager.TLSConfig()
+
+	tcpListener, err := net.ListenTCP("tcp", &net.TCPAddr{Port: 8080})


The acme code for sure used 443. Now that we're not bound to ACME, this could probably be configurable so you don't need CAP_NET_BIND_SERVICE capabilities when testing it out...

Signed-off-by: Guilherme Carvalho <[email protected]>

guilhermocc · 2023-06-22T21:18:05Z

@azdagron thank you very much for the review so far, I've addressed the last comments you made, could you review it again? 😄

Signed-off-by: Guilherme Carvalho <[email protected]>

azdagron · 2023-06-26T16:25:06Z

support/oidc-discovery-provider/README.md

+| `key_file_path`      | string   | required  | The private key file path, the file must contain PEM encoded data. |            |
+| `file_sync_interval` | duration | optional  | Controls how frequently the service polls the files for changes.   | 1 minute   |
+| `file_sync_interval` | duration | optional  | Controls how frequently the service polls the files for changes.   | 1 minute   |
+| `addr`               | string   | optional  | Exposes the service on the given address.                          | :433 |


443 should be the default (multiple places to fix)

Signed-off-by: Guilherme Carvalho <[email protected]>

azdagron

Thanks, @guilhermocc !

* Add disk cert manager Signed-off-by: Guilherme Carvalho <[email protected]> Signed-off-by: Guilherme Carvalho <[email protected]> Signed-off-by: Neniel <[email protected]>

guilhermocc added 5 commits May 30, 2023 13:31

Add disk cert manager

d7513e0

Signed-off-by: Guilherme Carvalho <[email protected]>

Add serving_cert_file config to oidc discovery provider in unix

9a8c675

Signed-off-by: Guilherme Carvalho <[email protected]>

Add serving cert file config for windows

cbef8cf

Signed-off-by: Guilherme Carvalho <[email protected]>

Fix update tests on linux

125d274

Signed-off-by: Guilherme Carvalho <[email protected]>

Fix rebase conflicts

e5e56a9

Signed-off-by: Guilherme Carvalho <[email protected]>

guilhermocc requested review from evan2645, amartinezfayo, azdagron, MarcosDY and rturner3 as code owners May 30, 2023 16:40

guilhermocc force-pushed the serving-cert-file-oidc-provider branch from a92394e to e5e56a9 Compare May 30, 2023 16:41

guilhermocc marked this pull request as draft May 30, 2023 16:41

guilhermocc added 2 commits May 30, 2023 14:10

Update documentation

67b20fb

Signed-off-by: Guilherme Carvalho <[email protected]>

Fix race condition

eddf53c

Signed-off-by: Guilherme Carvalho <[email protected]>

amartinezfayo assigned azdagron May 30, 2023

guilhermocc added 3 commits May 30, 2023 17:57

Fix lint in windows

91a7695

Signed-off-by: Guilherme Carvalho <[email protected]>

Fix timed assertions

c02a6da

Signed-off-by: Guilherme Carvalho <[email protected]>

Fix windows test

915b914

Signed-off-by: Guilherme Carvalho <[email protected]>

guilhermocc force-pushed the serving-cert-file-oidc-provider branch from 47c3661 to f146772 Compare May 31, 2023 17:53

Print cert manager logs on failed test

09acd52

Signed-off-by: Guilherme Carvalho <[email protected]>

guilhermocc force-pushed the serving-cert-file-oidc-provider branch from f146772 to 09acd52 Compare May 31, 2023 20:19

guilhermocc added 5 commits June 1, 2023 11:27

Remove fsnotify usage

fcba21a

Signed-off-by: Guilherme Carvalho <[email protected]>

Refactor tests

fb9bd71

Signed-off-by: Guilherme Carvalho <[email protected]>

Fix tests on windows

61d8e65

Signed-off-by: Guilherme Carvalho <[email protected]>

Fix file sync

e43b173

Signed-off-by: Guilherme Carvalho <[email protected]>

Make file sync interval configurable

af12dbe

Signed-off-by: Guilherme Carvalho <[email protected]>

guilhermocc force-pushed the serving-cert-file-oidc-provider branch from 284e928 to c7f1bdf Compare June 1, 2023 20:34

Refactor some comments

75123b3

Signed-off-by: Guilherme Carvalho <[email protected]>

guilhermocc force-pushed the serving-cert-file-oidc-provider branch from c7f1bdf to 75123b3 Compare June 1, 2023 20:41

guilhermocc marked this pull request as ready for review June 1, 2023 22:14

Address comments for windows platform

85abb9a

Signed-off-by: Guilherme Carvalho <[email protected]>

guilhermocc force-pushed the serving-cert-file-oidc-provider branch from 5872843 to 92e89f0 Compare June 9, 2023 20:35

Address comments to use context for ending goroutines

50eb322

Signed-off-by: Guilherme Carvalho <[email protected]>

guilhermocc force-pushed the serving-cert-file-oidc-provider branch from 92e89f0 to 50eb322 Compare June 9, 2023 21:21

Fix tests on windows

9f5da60

Signed-off-by: Guilherme Carvalho <[email protected]>

MarcosDY reviewed Jun 13, 2023

View reviewed changes

guilhermocc force-pushed the serving-cert-file-oidc-provider branch from 8743e34 to c541a57 Compare June 13, 2023 17:49

Address PR comments

3d36f1f

Signed-off-by: Guilherme Carvalho <[email protected]>

guilhermocc force-pushed the serving-cert-file-oidc-provider branch from c541a57 to 3d36f1f Compare June 13, 2023 20:04

azdagron reviewed Jun 20, 2023

View reviewed changes

Enable user to select the address for serving the https service

d47e082

Signed-off-by: Guilherme Carvalho <[email protected]>

Fix lint

8cacc0a

Signed-off-by: Guilherme Carvalho <[email protected]>

guilhermocc force-pushed the serving-cert-file-oidc-provider branch from b7baa15 to d4e615f Compare June 26, 2023 12:45

Fix windows tests

996e427

Signed-off-by: Guilherme Carvalho <[email protected]>

guilhermocc force-pushed the serving-cert-file-oidc-provider branch from d4e615f to 996e427 Compare June 26, 2023 13:12

azdagron reviewed Jun 26, 2023

View reviewed changes

Update default HTTPS port

dceafe0

Signed-off-by: Guilherme Carvalho <[email protected]>

guilhermocc force-pushed the serving-cert-file-oidc-provider branch from 4dde3f1 to dd04ac6 Compare June 30, 2023 18:48

Fix flaky tests

c18537e

Signed-off-by: Guilherme Carvalho <[email protected]>

guilhermocc force-pushed the serving-cert-file-oidc-provider branch from dd04ac6 to c18537e Compare June 30, 2023 20:57

guilhermocc requested a review from azdagron July 3, 2023 16:16

Remove duplicated documentation

c9c3612

Signed-off-by: Guilherme Carvalho <[email protected]>

azdagron added this to the 1.7.1 milestone Jul 3, 2023

azdagron approved these changes Jul 3, 2023

View reviewed changes

Merge branch 'main' into serving-cert-file-oidc-provider

4b2b816

rturner3 merged commit d1c58f8 into spiffe:main Jul 4, 2023


		tmpDir := t.TempDir()

		writeFile(t, tmpDir+keyFilePath, oidcServerKeyPem)


		tlsConfig := certManager.TLSConfig()

		tcpListener, err := net.ListenTCP("tcp", &net.TCPAddr{Port: 8080})

	[1]: One of `acme`, `serving_cert_file`, `listen_socket_path` must be defined.
	[1]: One of `acme`, `serving_cert_file`, or `listen_socket_path` must be defined.

Serving cert file OIDC provider #4190

Serving cert file OIDC provider #4190

Uh oh!

Conversation

guilhermocc commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guilhermocc Jun 13, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

guilhermocc commented Jun 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azdagron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

guilhermocc commented May 30, 2023 •

edited

Loading

guilhermocc Jun 13, 2023 •

edited

Loading