feat: Support HTTPS endpoints #1084
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ifindlay-cci
force-pushed
the
enable-opening-tls-endpoint
branch
4 times, most recently
from
ab9839c
to
5ccf512
Compare
ifindlay-cci
changed the title
When running as an app in CF we can rely on the platform to handle TLS setup, but on a VM currently there is no way to have encrypted traffic. TPCF-26820
ifindlay-cci
force-pushed
the
enable-opening-tls-endpoint
branch
from
5ccf512
to
290446f
Compare
it is angry about `.` imports. But we do not mind because this is not production code.
zucchinidev
reviewed
Sep 4, 2024
internal/testdrive/broker_start.go
Outdated
| scheme := "http" | ||
| for _, envVar := range cmd.Env { | ||
| if strings.HasPrefix(envVar, "TLS_") { | ||
| http.DefaultTransport.(*http.Transport).TLSClientConfig = &tls.Config{InsecureSkipVerify: true} |
There was a problem hiding this comment.
issue: Avoid modifying the global http.DefaultTransport directly. Instead, create a custom transport.
zucchinidev
requested changes
Sep 4, 2024
internal/testdrive/broker_start.go
Outdated
| caBytes, err := x509.CreateCertificate(rand.Reader, ca, ca, &caPrivKey.PublicKey, caPrivKey) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| caPEM, _ := encodeKeyPair(caBytes, x509.MarshalPKCS1PrivateKey(caPrivKey)) |
There was a problem hiding this comment.
issue (non-blocking): the PEM-encoded CA certificate is not being used. The code is not necessary, by removing it, we will improve the readability and the public API of the createCAKeyPair function
internal/testdrive/broker_start.go
Outdated
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| certPEM, certPrivKeyPEM := encodeKeyPair(certBytes, x509.MarshalPKCS1PrivateKey(certPrivKey)) | ||
| return certPEM, certPrivKeyPEM |
There was a problem hiding this comment.
thoughts: I prefer to declare the variable using value semantics and return it using pointer semantic. If the body of the functions grows, the readability gets worse.
return &certPEM, ...
There was a problem hiding this comment.
I'm not sure I understand how the readability will get better by returning a pointer?
cmd/serve.go
Outdated
| tlsKey := viper.GetString(tlsKeyProp) | ||
|
|
||
| logger.Info("tlsCertCaBundle", lager.Data{"tlsCertCaBundle": tlsCertCaBundle}) | ||
| logger.Info("tlsKey", lager.Data{"tlsKey": tlsKey}) |
There was a problem hiding this comment.
issue: we want to avoid logging sensitive data.
cmd/serve.go
Outdated
| logger.Fatal("Failed to start broker", err) | ||
| } | ||
| } else { | ||
| _ = httpServer.ListenAndServe() |
There was a problem hiding this comment.
Suggested change
| _ = httpServer.ListenAndServe() | |
| err := httpServer.ListenAndServe() | |
| if err != nil { | |
| logger.Fatal("Failed to start broker without TLS", err) | |
| } |
Co-authored-by: Andrea Zucchini <[email protected]>
currently the broker.client is not embedding http client. It never needed to because the broker was written as a CF app and therefore never had to consider handling self signed certs. But since we are changing towards supporting VM based deployment, we need to think about how our test client will handle self signed certs. This seems to be the path of least resistance, by embedding an http client in the broker client struct, we can rely on setting the embedded clients transport config to allow for "insecure" connecions.
nouseforaname
force-pushed
the
enable-opening-tls-endpoint
branch
from
019ba01
to
172bcab
Compare
FelisiaM
approved these changes
Sep 10, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist: