[SECURESIGN-994] Enable TLS on secureSign components: mount certificates/keys #483

fghanmi · 2024-06-28T09:14:28Z

Please do not merge, work in progress.

Depends on open issues:

Add option to enable TLS for communication with CTlog sigstore/fulcio#1717
Add option to enable TLS Security for CTLog server google/certificate-transparency-go#1522
Add option to enable TLS for communication with Trillian google/certificate-transparency-go#1524
Add option to enable TLS for communication with Trillian sigstore/rekor#2163

openshift-ci · 2024-06-28T09:14:38Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fghanmi
Once this PR has been reviewed and has the lgtm label, please assign sallyom for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2024-06-30T16:12:11Z

@fghanmi: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/tas-operator-e2e	`c06886f`	link	true	`/test tas-operator-e2e`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

osmman

I have a few comments on this PR. First, I recommend dividing the PR into separate components. Please test each component to see if it can use TLS, because it looks like some of them are unable to do that. If a component cannot use TLS, it is unnecessary to modify the operator for that component. Additionally, please create unit and end-to-end tests.

osmman · 2024-07-02T06:55:34Z

api/v1alpha1/ctlog_types.go

+
+	// Secret with TLS server certificates and private key
+	//+optional
+	TLSCertificate TLSCert `json:"tlsCertificate"`


Suggested change

TLSCertificate TLSCert `json:"tlsCertificate"`

TLSCertificate TLSCert `json:"tls"`

osmman · 2024-07-02T06:57:09Z

api/v1alpha1/ctlog_types.go

@@ -46,6 +50,7 @@ type CTlogStatus struct {
 	PrivateKeyPasswordRef *SecretKeySelector    `json:"privateKeyPasswordRef,omitempty"`
 	PublicKeyRef          *SecretKeySelector    `json:"publicKeyRef,omitempty"`
 	RootCertificates      []SecretKeySelector   `json:"rootCertificates,omitempty"`
+	TLSCertificate        *TLSCert              `json:"tlsCertificate,omitempty"`


Suggested change

TLSCertificate *TLSCert `json:"tlsCertificate,omitempty"`

TLSCertificate *TLSCert `json:"tls,omitempty"`

osmman · 2024-07-02T06:57:32Z

api/v1alpha1/fulcio_types.go

@@ -21,6 +21,9 @@ type FulcioSpec struct {
 	// ConfigMap with additional bundle of trusted CA
 	//+optional
 	TrustedCA *LocalObjectReference `json:"trustedCA,omitempty"`
+	// Secret with TLS server certificates and private key
+	//+optional
+	TLSCertificate TLSCert `json:"tlsCertificate"`


Suggested change

TLSCertificate TLSCert `json:"tlsCertificate"`

TLSCertificate TLSCert `json:"tls"`

osmman · 2024-07-02T06:57:42Z

api/v1alpha1/fulcio_types.go

@@ -96,6 +99,7 @@ type OIDCIssuer struct {
 type FulcioStatus struct {
 	ServerConfigRef *LocalObjectReference `json:"serverConfigRef,omitempty"`
 	Certificate     *FulcioCert           `json:"certificate,omitempty"`
+	TLSCertificate  *TLSCert              `json:"tlsCertificate,omitempty"`


Suggested change

TLSCertificate *TLSCert `json:"tlsCertificate,omitempty"`

TLSCertificate *TLSCert `json:"tls,omitempty"`

osmman · 2024-07-02T06:58:30Z

api/v1alpha1/trillian_types.go

+type TrillianServer struct {
+	// Secret with TLS server certificate and private key
+	//+optional
+	TLSCertificate TLSCert `json:"tlsCertificate"`


Suggested change

TLSCertificate TLSCert `json:"tlsCertificate"`

TLSCertificate TLSCert `json:"tls"`

osmman · 2024-07-02T06:58:39Z

api/v1alpha1/trillian_types.go

+type TrillianSigner struct {
+	// Secret with TLS server certificate and private key
+	//+optional
+	TLSCertificate TLSCert `json:"tlsCertificate"`


Suggested change

TLSCertificate TLSCert `json:"tlsCertificate"`

TLSCertificate TLSCert `json:"tls"`

osmman · 2024-07-02T07:29:21Z

internal/controller/ctlog/actions/deployment.go

+				MountPath: "/etc/ssl/certs",
+				ReadOnly:  true,
+			})
+		dp.Spec.Template.Spec.Containers[0].Args = append(dp.Spec.Template.Spec.Containers[0].Args, "--tls-certificate", "/etc/ssl/certs/tls.crt")


Where did you find these options? There is no option to configure TLSConfig in ct_server application

https://github.com/securesign/certificate-transparency-go/blob/main/trillian/ctfe/ct_server/main.go#L309

This PR won't be merged until the issues on upstreams are solved (mentioned here)
I've prepared 4 PRs to enable TLS on CTLog, rekor, fulcio
tls-certificate option is here: https://github.com/google/certificate-transparency-go/pull/1523/files#diff-7a183747e4fb8d227a2b6b60050cbab942013815105b231c7dedf4d0e9fbabbbR61

osmman · 2024-07-02T07:39:10Z

internal/controller/fulcio/actions/deployment.go

+			})
+		dp.Spec.Template.Spec.Containers[0].Args = append(dp.Spec.Template.Spec.Containers[0].Args, "--grpc-tls-certificate", "/etc/ssl/certs/tls.crt")
+		dp.Spec.Template.Spec.Containers[0].Args = append(dp.Spec.Template.Spec.Containers[0].Args, "--grpc-tls-key", "/etc/ssl/certs/tls.key")
+		dp.Spec.Template.Spec.Containers[0].Args = append(dp.Spec.Template.Spec.Containers[0].Args, "--tls-ca-cert", "/etc/ssl/certs/ca.crt")


Where did you find this option on Fulcio? I unable to find any reference in it's source code.

same, still on PR
https://github.com/sigstore/fulcio/pull/1718/files#diff-1627906dbce532aff8cc81f54790572e5ba04f3e568c88b6ed39a590642dafa4R112

openshift-merge-robot · 2024-07-04T10:41:19Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Enable TLS: mount signed certificates

bf627ba

openshift-ci bot requested review from sabre1041 and tommyd450 June 28, 2024 09:14

fghanmi added the do-not-merge/work-in-progress label Jun 28, 2024

fghanmi changed the title ~~[In progress] [SECURESIGN-994] Enable TLS: mount signed certificates~~ [SECURESIGN-994] Enable TLS: mount signed certificates Jun 28, 2024

openshift-ci bot removed the do-not-merge/work-in-progress label Jun 28, 2024

fghanmi added the do-not-merge/work-in-progress label Jun 28, 2024

fghanmi added 4 commits June 28, 2024 15:17

fix e2e update test

26986c4

fix e2e

56a6fe8

fix e2e update test

bb9f673

updates

c06886f

openshift-ci bot removed the do-not-merge/work-in-progress label Jul 1, 2024

fghanmi changed the title ~~[SECURESIGN-994] Enable TLS: mount signed certificates~~ [SECURESIGN-994] Enable TLS on secureSign components: mount certificates/keys Jul 1, 2024

osmman requested changes Jul 2, 2024

View reviewed changes

openshift-ci bot assigned osmman Jul 2, 2024

osmman marked this pull request as draft July 2, 2024 08:36

openshift-ci bot added the do-not-merge/work-in-progress label Jul 2, 2024

openshift-merge-robot added the needs-rebase label Jul 4, 2024

fghanmi closed this Sep 2, 2024

osmman deleted the SECURESIGN-994 branch September 17, 2024 12:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SECURESIGN-994] Enable TLS on secureSign components: mount certificates/keys #483

[SECURESIGN-994] Enable TLS on secureSign components: mount certificates/keys #483

fghanmi commented Jun 28, 2024 •

edited

Loading

openshift-ci bot commented Jun 28, 2024

openshift-ci bot commented Jun 30, 2024

osmman left a comment

osmman Jul 2, 2024

osmman Jul 2, 2024

osmman Jul 2, 2024

osmman Jul 2, 2024

osmman Jul 2, 2024

osmman Jul 2, 2024

osmman Jul 2, 2024

fghanmi Jul 2, 2024 •

edited

Loading

osmman Jul 2, 2024

fghanmi Jul 2, 2024

openshift-merge-robot commented Jul 4, 2024

	TLSCertificate TLSCert `json:"tlsCertificate"`
	TLSCertificate TLSCert `json:"tls"`

	TLSCertificate *TLSCert `json:"tlsCertificate,omitempty"`
	TLSCertificate *TLSCert `json:"tls,omitempty"`

[SECURESIGN-994] Enable TLS on secureSign components: mount certificates/keys #483

[SECURESIGN-994] Enable TLS on secureSign components: mount certificates/keys #483

Conversation

fghanmi commented Jun 28, 2024 • edited Loading

openshift-ci bot commented Jun 28, 2024

openshift-ci bot commented Jun 30, 2024

osmman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fghanmi Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-merge-robot commented Jul 4, 2024

fghanmi commented Jun 28, 2024 •

edited

Loading

fghanmi Jul 2, 2024 •

edited

Loading