[SECURESIGN-994] Add TLS to Fulcio and CTlog services #492

fghanmi · 2024-07-04T14:47:54Z

No description provided.

openshift-ci · 2024-07-04T14:48:04Z

[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 osmman 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

osmman · 2024-09-12T13:33:52Z

api/v1alpha1/ctlog_types.go

@@ -48,6 +48,9 @@ type CTlogSpec struct {
 	// publicKeyRef, rootCertificates and trillian will be overridden.
 	//+optional
 	ServerConfigRef *LocalObjectReference `json:"serverConfigRef,omitempty"`
+	// Configuration for enabling TLS (Transport Layer Security) encryption for manged database.


please fix doc, it is configuration to encrypt CTlog server

bouskaJ · 2024-09-13T06:55:28Z

bundle/manifests/rhtas-operator.clusterserviceversion.yaml

@@ -309,7 +309,7 @@ metadata:
    features.operators.openshift.io/token-auth-azure: "false"
    features.operators.openshift.io/token-auth-gcp: "false"
    operators.openshift.io/valid-subscription: '["Red Hat Trusted Artifact Signer"]'
-    operators.operatorframework.io/builder: operator-sdk-v1.34.2
+    operators.operatorframework.io/builder: operator-sdk-v1.34.1


do not change operator-sdk version

bouskaJ · 2024-09-13T06:57:41Z

api/v1alpha1/common.go

 	//+kubebuilder:validation:Maximum:=65535
-	//+kubebuilder:default:=80
+	//+kubebuilder:default:=0


I don't think that default port 0 is the right choice. I would prefer to omit this value (use nil) in case it is not used.

bouskaJ · 2024-09-13T07:05:21Z

internal/controller/fulcio/actions/deployment.go

-		instance.Spec.Ctlog.Address = fmt.Sprintf("http://ctlog.%s.svc", instance.Namespace)
-	case instance.Spec.Ctlog.Port == nil:
-		port := int32(80)
+	if instance.Spec.Ctlog.Address == "" {


instance.Spec.TrustedCA != nil does not necessary means that you have TLS enabled CTLog service. I would try to load Ctlog service and try to get scheme and port from it.

So, Fulcio needs to depend on CTlog and should only start after CTlog has started, in order to fetch its service. Is that correct?

there is no dependency on CTLog itself. You can use label selector to find the service

your deployment depends on ctlog.namespace.svc" anyway so your fulcio Fulcio will not become fully ready until Ctlog starts. The only downside of this solution is that Fulcio would wait for Ctlog service to be created but I don't think that it is a big problem.

@osmman WDYT?

@bouskaJ , is it better now ?

I wold do it like this

var url if instance.Spec.Ctlog.Address != "" && instance.Spec.Ctlog.Port != "" { url = // use specified } else { svc := FindService(ctx, cli, LabelsForComponent(ctl.ComponentName, instance.Name) // retrieve protocol from service port // compose url url = fmt.Sprintf("%s%s.svc:%d", svc.Name, svc.Namespace, svc.Port) } url += instance.Spec.Ctlog.Preffix

I hope the above snapshot is clear enough.

please do not depends on services, you need to keep support of independent deployment so ctlog and fulcio could be deployed separatly (different namespace, cluster) and Fulcio will sucessfuly start without runnig CTlog only create certificate requests will be rejected which is intended behavior.

So please try to use only information which are present in Fulcio resource

we have TrustedCA, but as Jan said, it does not necessarily mean that CTlog has TLS enabled..
that's why in the beginning, I was using a dedicated field for that TLS.CaCert, I used it to decide whether fulcio should use TLS or not.

please do not depends on services, you need to keep support of independent deployment so ctlog and fulcio could be deployed separatly (different namespace, cluster) and Fulcio will sucessfuly start without runnig CTlog only create certificate requests will be rejected which is intended behavior.

So please try to use only information which are present in Fulcio resource

I was thinking about it like a fail-over solution in case there is no information in Fulcio resource. With Firas solution, you only think that you should use tls (based on env) but the running CTL does not need to be TLS secured at all. You can get much more valid information from service itself. Service lookup (based on labels) does not create any hard dependency and we already use label selectors in the TUF key auto-discovery.

bouskaJ · 2024-09-13T07:08:07Z

internal/controller/fulcio/actions/deployment.go

+			})
+			return i.FailedWithStatusUpdate(ctx, fmt.Errorf("could not get CA path: %w", err), instance)
+		}
+		dp.Spec.Template.Spec.Containers[0].Args = append(dp.Spec.Template.Spec.Containers[0].Args, "--ct-log.tls-ca-cert", caPath)


not even Fulcio is able to use SSL_CERT_DIR variable?

the only TLS option I see is this one:
https://github.com/sigstore/fulcio/blob/main/cmd/app/serve.go#L112

What I see in the implementation, the server should work with system certs. Give it a try, please. https://github.com/securesign/fulcio/blob/main/cmd/app/serve.go#L279-L302

Fulcio.Spec.TrustedCA does not necessarily mean that you specify CA for CTLog -> I would prefer to mount it as system CA

I was able to make it working with system-certs. This is my PoC bouskaJ@b9f4154

@bouskaJ I've cherry-picked your suggestion, and tested it, all works fine! Thanks!

bouskaJ · 2024-09-13T12:57:20Z

internal/controller/common/utils/kubernetes/service.go

+func GetService(client client.Client, namespace, serviceName string) (*corev1.Service, error) {
+	var service corev1.Service
+
+	err := client.Get(context.TODO(), types.NamespacedName{


do not create new context. Pass existing one.

bouskaJ · 2024-09-13T13:02:01Z

internal/controller/fulcio/actions/deployment.go

+			})
+			return i.FailedWithStatusUpdate(ctx, fmt.Errorf("could not get CA path: %w", err), instance)
+		}
+		dp.Spec.Template.Spec.Containers[0].Args = append(dp.Spec.Template.Spec.Containers[0].Args, "--ct-log.tls-ca-cert", caPath)


Fulcio.Spec.TrustedCA does not necessarily mean that you specify CA for CTLog -> I would prefer to mount it as system CA

bouskaJ · 2024-09-13T13:08:27Z

internal/controller/fulcio/actions/deployment.go

-		instance.Spec.Ctlog.Address = fmt.Sprintf("http://ctlog.%s.svc", instance.Namespace)
-	case instance.Spec.Ctlog.Port == nil:
-		port := int32(80)
+	if instance.Spec.Ctlog.Address == "" {


I wold do it like this

var url if instance.Spec.Ctlog.Address != "" && instance.Spec.Ctlog.Port != "" { url = // use specified } else { svc := FindService(ctx, cli, LabelsForComponent(ctl.ComponentName, instance.Name) // retrieve protocol from service port // compose url url = fmt.Sprintf("%s%s.svc:%d", svc.Name, svc.Namespace, svc.Port) } url += instance.Spec.Ctlog.Preffix

I hope the above snapshot is clear enough.

openshift-merge-robot · 2024-09-18T08:18:54Z

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.

openshift-ci bot requested review from lkatalin and tommyd450 July 4, 2024 14:48

fghanmi added the do-not-merge/hold label Jul 4, 2024

fghanmi force-pushed the TLSSuppportFulcioCTlog branch from b387080 to 7092e63 Compare July 5, 2024 14:08

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

fghanmi force-pushed the TLSSuppportFulcioCTlog branch from df48e12 to e9ef614 Compare August 3, 2024 09:14

openshift-merge-robot added needs-rebase and removed needs-rebase labels Aug 3, 2024

fghanmi force-pushed the TLSSuppportFulcioCTlog branch from 77ab1ce to 1219155 Compare August 17, 2024 10:35

openshift-merge-robot removed the needs-rebase label Aug 17, 2024

fghanmi removed the do-not-merge/hold label Aug 17, 2024

fghanmi changed the title ~~Add TLS to Fulcio and CTlog services~~ [SECURESIGN-994] Add TLS to Fulcio and CTlog services Aug 26, 2024

fghanmi force-pushed the TLSSuppportFulcioCTlog branch from 56b68b4 to b2a2989 Compare August 31, 2024 22:33

fghanmi added 5 commits September 12, 2024 09:27

Add TLS to Rekor and Trillian services

97e06d1

updates-1

3100639

fix conflicts

0e8373f

resolve conflicts+updates

4dd44ad

updates

1943664

fghanmi force-pushed the TLSSuppportFulcioCTlog branch from b2a2989 to 1943664 Compare September 12, 2024 07:37

fghanmi added 5 commits September 12, 2024 13:16

refactor code

73a3ee4

revert v1alpha1_securesign.yaml

2bd7784

update

35065b2

update

65ebbc7

update

e400331

fghanmi requested review from osmman and bouskaJ and removed request for lkatalin and tommyd450 September 12, 2024 11:22

osmman reviewed Sep 12, 2024

View reviewed changes

fghanmi added 2 commits September 12, 2024 15:37

fix comments

ba8516c

update manifests

9baa3a1

bouskaJ mentioned this pull request Sep 13, 2024

[SECURESIGN-994] Add TLS to Rekor and Trillian services #493

Open

bouskaJ reviewed Sep 13, 2024

View reviewed changes

fghanmi added 3 commits September 13, 2024 13:34

updates: review

93104c1

revert operator-sdk version

c0abda6

fix fulcio tests

ed50271

bouskaJ reviewed Sep 13, 2024

View reviewed changes

bouskaJ and others added 3 commits September 16, 2024 16:31

jbouska - test

67edfb8

updates

ef02856

fix golangci-lint

17aeae0

openshift-merge-robot added the needs-rebase label Sep 18, 2024

osmman added this to the 1.2.0 milestone Sep 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SECURESIGN-994] Add TLS to Fulcio and CTlog services #492

[SECURESIGN-994] Add TLS to Fulcio and CTlog services #492

fghanmi commented Jul 4, 2024 •

edited

Loading

openshift-ci bot commented Jul 4, 2024

osmman Sep 12, 2024

bouskaJ Sep 13, 2024

bouskaJ Sep 13, 2024

bouskaJ Sep 13, 2024

fghanmi Sep 13, 2024

bouskaJ Sep 13, 2024

fghanmi Sep 13, 2024

bouskaJ Sep 13, 2024

osmman Sep 13, 2024

fghanmi Sep 13, 2024

bouskaJ Sep 16, 2024 •

edited

Loading

bouskaJ Sep 13, 2024

fghanmi Sep 13, 2024

bouskaJ Sep 13, 2024

bouskaJ Sep 13, 2024

bouskaJ Sep 16, 2024

fghanmi Sep 18, 2024 •

edited

Loading

bouskaJ Sep 13, 2024

bouskaJ Sep 13, 2024

bouskaJ Sep 13, 2024

openshift-merge-robot commented Sep 18, 2024

[SECURESIGN-994] Add TLS to Fulcio and CTlog services #492

Are you sure you want to change the base?

[SECURESIGN-994] Add TLS to Fulcio and CTlog services #492

Conversation

fghanmi commented Jul 4, 2024 • edited Loading

openshift-ci bot commented Jul 4, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bouskaJ Sep 16, 2024 • edited Loading

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 Sep 18, 2024 • edited Loading

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

openshift-merge-robot commented Sep 18, 2024

fghanmi commented Jul 4, 2024 •

edited

Loading

bouskaJ Sep 16, 2024 •

edited

Loading

fghanmi Sep 18, 2024 •

edited

Loading