Skip to content

Conversation

@fghanmi
Copy link
Member

@fghanmi fghanmi commented Dec 8, 2025

PR Type

Enhancement


Description

  • Add configurable service URLs for Ctlog, Fulcio, Rekor, and TSA in TUF repository

  • Introduce service configuration structs with address and port fields

  • Implement URL resolution logic supporting both custom and default service endpoints

  • Update TUF initialization job to pass resolved service URIs as arguments

  • Add helper method to retrieve trusted CA from annotations


Diagram Walkthrough

flowchart LR
  A["TufSpec"] -->|adds service configs| B["CtlogService<br/>FulcioService<br/>RekorService<br/>TsaService"]
  B -->|resolved by| C["resolveServicesUrls"]
  C -->|generates| D["ServicesURIs"]
  D -->|passed to| E["TufInitJob"]
  E -->|as CLI args| F["TUF initialization"]
Loading

File Walkthrough

Relevant files
Enhancement
common.go
Add service configuration structs                                               

api/v1alpha1/common.go

  • Add three new service configuration structs: FulcioService,
    RekorService, and TsaService
  • Each struct contains optional Address and Port fields with validation
    (port range 1-65535)
  • Structs follow the same pattern as existing CtlogService for
    consistency
+36/-0   
tuf_types.go
Add service configs to TufSpec                                                     

api/v1alpha1/tuf_types.go

  • Add four service configuration fields to TufSpec: Ctlog, Fulcio,
    Rekor, and Tsa
  • Add GetTrustedCA() method to retrieve trusted CA from annotations
  • Update Ctlog field with default prefix value
+23/-0   
tuf_init_job.go
Implement service URL resolution logic                                     

internal/controller/tuf/utils/tuf_init_job.go

  • Add ServicesURIs struct to hold resolved service URLs
  • Implement resolveServicesUrls() function to resolve service endpoints
    from spec or defaults
  • Support custom addresses/ports or in-cluster service discovery with
    protocol selection
  • Update EnsureTufInitJob to resolve and pass service URIs as CLI
    arguments to TUF init job
  • Add imports for service actions and TLS utilities
+83/-0   
Code generation
zz_generated.deepcopy.go
Generate deepcopy methods for services                                     

api/v1alpha1/zz_generated.deepcopy.go

  • Auto-generate DeepCopyInto and DeepCopy methods for FulcioService,
    RekorService, and TsaService
  • Update TufSpec.DeepCopyInto to handle new service configuration fields
  • Fix RekorSpec.DeepCopyInto to properly deep copy Monitoring field
  • Update MonitoringWithTLogConfig.DeepCopyInto to deep copy Tuf field
+66/-2   
Configuration changes
rhtas.redhat.com_securesigns.yaml
Update SecureSign CRD schema                                                         

config/crd/bases/rhtas.redhat.com_securesigns.yaml

  • Add CRD schema definitions for ctlog, fulcio, rekor, and tsa service
    configurations
  • Each service config includes optional address and port fields with
    validation
  • Ctlog includes prefix field with default and pattern validation
+61/-0   
rhtas.redhat.com_tufs.yaml
Update Tuf CRD schema                                                                       

config/crd/bases/rhtas.redhat.com_tufs.yaml

  • Add CRD schema definitions for ctlog, fulcio, rekor, and tsa service
    configurations
  • Each service config includes optional address and port fields with
    validation
  • Ctlog includes prefix field with default and pattern validation
+61/-0   
Dependencies
images.env
Update TUF image reference                                                             

config/default/images.env

  • Update RELATED_IMAGE_TUF to new image SHA256 digest
+1/-1     

@qodo-code-review
Copy link

qodo-code-review bot commented Dec 8, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Insecure URL construction

Description: When a custom service address is provided (Fulcio/Rekor/TSA), the code builds URLs without
enforcing or validating the scheme, and for Ctlog it also omits the scheme and appends the
user-controlled 'prefix' path directly; this can lead to ambiguous or malformed endpoints,
potential SSRF if untrusted inputs reach spec, and TLS being silently bypassed (e.g.,
"http://..." used even when 'tls.UseTlsClient' would imply HTTPS).
tuf_init_job.go [137-168]

Referred Code
// Fulcio
if instance.Spec.Fulcio.Address != "" {
	url := instance.Spec.Fulcio.Address
	if instance.Spec.Fulcio.Port != nil {
		url = fmt.Sprintf("%s:%d", url, *instance.Spec.Fulcio.Port)
	}
	uris.Fulcio = url
} else {
	uris.Fulcio = fmt.Sprintf("%s://%s.%s.svc", protocol, fulcio.DeploymentName, instance.Namespace)
}

// Rekor
if instance.Spec.Rekor.Address != "" {
	url := instance.Spec.Rekor.Address
	if instance.Spec.Rekor.Port != nil {
		url = fmt.Sprintf("%s:%d", url, *instance.Spec.Rekor.Port)
	}
	uris.Rekor = url
} else {
	uris.Rekor = fmt.Sprintf("%s://%s.%s.svc", protocol, rekor.ServerDeploymentName, instance.Namespace)
}


 ... (clipped 11 lines)
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
No Audit Logs: The newly added service URL resolution and job argument construction perform critical
configuration actions without emitting any audit logs of decisions or outcomes.

Referred Code
func EnsureTufInitJob(instance *rhtasv1alpha1.Tuf, sa string, labels map[string]string) func(*batchv1.Job) error {
	return func(job *batchv1.Job) error {

		// prepare args
		servicesURIs, err := resolveServicesUrls(instance)
		if err != nil {
			return fmt.Errorf("could not resolve services urls: %w", err)
		}
		args := []string{"--export-keys", instance.Spec.RootKeySecretRef.Name}
		for _, key := range instance.Spec.Keys {
			switch key.Name {
			case "rekor.pub":
				args = append(args, "--rekor-key", filepath.Join(secretsMonthPath, key.Name))
				args = append(args, "--rekor-uri", servicesURIs.Rekor)
			case "ctfe.pub":
				args = append(args, "--ctlog-key", filepath.Join(secretsMonthPath, key.Name))
				args = append(args, "--ctlog-uri", servicesURIs.Ctlog)
			case "fulcio_v1.crt.pem":
				args = append(args, "--fulcio-cert", filepath.Join(secretsMonthPath, key.Name))
				args = append(args, "--fulcio-uri", servicesURIs.Fulcio)
			case "tsa.certchain.pem":


 ... (clipped 115 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Edge Cases Missing: URL building does not validate or normalize provided addresses (e.g., missing scheme,
trailing slashes) and only checks Ctlog prefix, potentially leading to malformed URIs
without clear handling.

Referred Code
func resolveServicesUrls(instance *rhtasv1alpha1.Tuf) (ServicesURIs, error) {
	uris := ServicesURIs{}
	var (
		protocol string
	)
	if tls.UseTlsClient(instance) {
		protocol = "https"
	} else {
		protocol = "http"
	}

	// Ctlog
	if instance.Spec.Ctlog.Prefix == "" {
		return ServicesURIs{}, futils.ErrCtlogPrefixNotSpecified
	}

	if instance.Spec.Ctlog.Address != "" {
		url := instance.Spec.Ctlog.Address
		if instance.Spec.Ctlog.Port != nil {
			url = fmt.Sprintf("%s:%d", url, *instance.Spec.Ctlog.Port)
		}


 ... (clipped 39 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Input Validation: External spec inputs for service addresses and ports are concatenated into URIs without
explicit validation or scheme enforcement, which could allow invalid or unsafe endpoints.

Referred Code
// Ctlog
if instance.Spec.Ctlog.Prefix == "" {
	return ServicesURIs{}, futils.ErrCtlogPrefixNotSpecified
}

if instance.Spec.Ctlog.Address != "" {
	url := instance.Spec.Ctlog.Address
	if instance.Spec.Ctlog.Port != nil {
		url = fmt.Sprintf("%s:%d", url, *instance.Spec.Ctlog.Port)
	}
	uris.Ctlog = fmt.Sprintf("%s/%s", url, instance.Spec.Ctlog.Prefix)
} else {
	uris.Ctlog = fmt.Sprintf("%s://%s.%s.svc/%s", protocol, ctlog.DeploymentName, instance.Namespace, instance.Spec.Ctlog.Prefix)
}

// Fulcio
if instance.Spec.Fulcio.Address != "" {
	url := instance.Spec.Fulcio.Address
	if instance.Spec.Fulcio.Port != nil {
		url = fmt.Sprintf("%s:%d", url, *instance.Spec.Fulcio.Port)
	}


 ... (clipped 26 lines)

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

qodo-code-review bot commented Dec 8, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Prepend protocol to custom service URIs

Prepend the protocol scheme (http:// or https://) to custom service URIs in
resolveServicesUrls to ensure they are valid.

internal/controller/tuf/utils/tuf_init_job.go [122-168]

 	// Ctlog
 	if instance.Spec.Ctlog.Prefix == "" {
 		return ServicesURIs{}, futils.ErrCtlogPrefixNotSpecified
 	}
 
 	if instance.Spec.Ctlog.Address != "" {
 		url := instance.Spec.Ctlog.Address
 		if instance.Spec.Ctlog.Port != nil {
 			url = fmt.Sprintf("%s:%d", url, *instance.Spec.Ctlog.Port)
 		}
-		uris.Ctlog = fmt.Sprintf("%s/%s", url, instance.Spec.Ctlog.Prefix)
+		uris.Ctlog = fmt.Sprintf("%s://%s/%s", protocol, url, instance.Spec.Ctlog.Prefix)
 	} else {
 		uris.Ctlog = fmt.Sprintf("%s://%s.%s.svc/%s", protocol, ctlog.DeploymentName, instance.Namespace, instance.Spec.Ctlog.Prefix)
 	}
 
 	// Fulcio
 	if instance.Spec.Fulcio.Address != "" {
 		url := instance.Spec.Fulcio.Address
 		if instance.Spec.Fulcio.Port != nil {
 			url = fmt.Sprintf("%s:%d", url, *instance.Spec.Fulcio.Port)
 		}
-		uris.Fulcio = url
+		uris.Fulcio = fmt.Sprintf("%s://%s", protocol, url)
 	} else {
 		uris.Fulcio = fmt.Sprintf("%s://%s.%s.svc", protocol, fulcio.DeploymentName, instance.Namespace)
 	}
 
 	// Rekor
 	if instance.Spec.Rekor.Address != "" {
 		url := instance.Spec.Rekor.Address
 		if instance.Spec.Rekor.Port != nil {
 			url = fmt.Sprintf("%s:%d", url, *instance.Spec.Rekor.Port)
 		}
-		uris.Rekor = url
+		uris.Rekor = fmt.Sprintf("%s://%s", protocol, url)
 	} else {
 		uris.Rekor = fmt.Sprintf("%s://%s.%s.svc", protocol, rekor.ServerDeploymentName, instance.Namespace)
 	}
 
 	// TSA
 	if instance.Spec.Tsa.Address != "" {
 		url := instance.Spec.Tsa.Address
 		if instance.Spec.Tsa.Port != nil {
 			url = fmt.Sprintf("%s:%d", url, *instance.Spec.Tsa.Port)
 		}
-		uris.Tsa = url
+		uris.Tsa = fmt.Sprintf("%s://%s", protocol, url)
 	} else {
 		uris.Tsa = fmt.Sprintf("%s://%s.%s.svc", protocol, tsa.DeploymentName, instance.Namespace)
 	}
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a bug where custom service URIs are generated without a protocol scheme, which would cause the TUF initialization job to fail.

High
General
Correct inaccurate struct comments

Correct the comments for RekorService and TsaService to refer to the correct
services (Rekor and TSA) instead of Fulcio.

api/v1alpha1/common.go [106-128]

-// RekorService configuration to connect Fulcio server
+// RekorService configuration to connect Rekor server
 type RekorService struct {
 	// Address to Rekor End point
 	//+optional
 	Address string `json:"address,omitempty"`
 	// Port of Rekor End point
 	//+kubebuilder:validation:Minimum:=1
 	//+kubebuilder:validation:Maximum:=65535
 	//+optional
 	Port *int32 `json:"port,omitempty"`
 }
 
-// TsaService configuration to connect Fulcio server
+// TsaService configuration to connect TSA server
 type TsaService struct {
 	// Address to TSA End point
 	//+optional
 	Address string `json:"address,omitempty"`
 	// Port of TSA End point
 	//+kubebuilder:validation:Minimum:=1
 	//+kubebuilder:validation:Maximum:=65535
 	//+optional
 	Port *int32 `json:"port,omitempty"`
 }
  • Apply / Chat
Suggestion importance[1-10]: 3

__

Why: The suggestion correctly points out misleading comments that were likely copy-pasted, improving code clarity and maintainability.

Low
  • Update

@securesign securesign deleted a comment from qodo-code-review bot Dec 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant