Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(fulcio/add-env): Add additional env variables #530

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat(fulcio/add-env): Add additional env variables #530

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
af3fc29
feat(fulcio/add-env): Add additional env variables
saisatishkarra May 18, 2023
bef4aba
fix(helm): make env variable optional
saisatishkarra May 19, 2023
a03c22a
modify env variable type
saisatishkarra May 21, 2023
1012833
improve naming scheme for extrernal credential config
saisatishkarra May 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion charts/fulcio/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ description: |

type: application

version: 2.3.2
version: 2.4.2
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
version: 2.4.2
version: 2.4.0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

appVersion: 1.3.1

keywords:
Expand Down
4 changes: 3 additions & 1 deletion charts/fulcio/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

<!-- This README.md is generated. Please edit README.md.gotmpl -->

![Version: 2.3.2](https://img.shields.io/badge/Version-2.3.2-informational?style=flat-square) ![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square) ![AppVersion: 1.3.1](https://img.shields.io/badge/AppVersion-1.3.1-informational?style=flat-square)
![Version: 2.4.2](https://img.shields.io/badge/Version-2.4.2-informational?style=flat-square) ![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square) ![AppVersion: 1.3.1](https://img.shields.io/badge/AppVersion-1.3.1-informational?style=flat-square)

Fulcio is a free code signing Certificate Authority, built to make short-lived certificates available to anyone.

Expand Down Expand Up @@ -109,12 +109,14 @@ helm uninstall [RELEASE_NAME]
| namespace.name | string | `"fulcio-system"` | |
| server.args.aws_hsm_root_ca_path | string | `nil` | |
| server.args.certificateAuthority | string | `"fileca"` | |
| server.args.creds | string | `""` | |
| server.args.ct_log_url | string | `""` | |
| server.args.disable_ct_log | bool | `false` | |
| server.args.gcp_private_ca_parent | string | `"projects/test/locations/us-east1/caPools/test"` | |
| server.args.grpcPort | int | `5554` | |
| server.args.hsm_caroot_id | string | `nil` | |
| server.args.port | int | `5555` | |
| server.env.GOOGLE_APPLICATION_CREDENTIALS | string | `"/etc/fulcio-config/cloud_credentials"` | |
saisatishkarra marked this conversation as resolved.
Show resolved Hide resolved
| server.grpcSvcPort | int | `5554` | |
| server.image.pullPolicy | string | `"IfNotPresent"` | |
| server.image.registry | string | `"gcr.io"` | |
Expand Down
1 change: 1 addition & 0 deletions charts/fulcio/templates/fulcio-configmap.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,4 @@ data:
{{- if (eq .Values.server.args.certificateAuthority "kmsca")}}
chain.pem: {{.Values.server.args.kms_cert_chain | quote }}
{{- end }}
cloud_credentials: {{.Values.server.args.creds | quote }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a conditional to avoid including when not specified

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added condition!

27 changes: 20 additions & 7 deletions charts/fulcio/templates/fulcio-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,27 @@ spec:
- "--kms-cert-chain-path=/etc/fulcio-config/chain.pem"
{{- end }}
- "--ct-log-url={{ if .Values.server.args.disable_ct_log }}{{ else if .Values.server.args.ct_log_url }}{{ .Values.server.args.ct_log_url }}{{ else }}http://{{ .Values.ctlog.name }}.{{ .Values.ctlog.namespace.name }}.svc/{{ .Values.ctlog.createctconfig.logPrefix }}{{ end }}"
{{- if eq .Values.server.args.certificateAuthority "fileca" }}
{{- if .Values.server.env }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesnt have a closing end tag, yet strangely no error is thrown

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The closing {{- end}} for line 64 is at line82

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it

To simplify, this condition should be an or statement that checks the presence of Values.server.env or whether Values.server.args.certificateAuthority == fileca. This would avoid the redundant PASSWORD env variable definition

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the or condition between Values.server.env and Values.server.args.certificateAuthority == fileca would work because there might be a case where there are env variables specified as key-value pairs and the certificateAuthority == "<something not fileca>" at which point it would still try to populate the PASSWORD as a secret ref that is NOT optional and the pod might fail / retry due to lack of missing secret.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is still fine. having the proposed conditional would still suffice. this conditional would remain in order to capture when it was defined, otherwise only the key/values would be captured. The contents within this conditional block can be removed as its no longer needed

env:
- name: PASSWORD
valueFrom:
secretKeyRef:
name: {{ .Values.server.secret }}
key: password
{{- end }}
{{- range $key, $value := .Values.server.env }}
- name: "{{ $key }}"
value: "{{ $value }}"
{{- end }}
{{- if eq .Values.server.args.certificateAuthority "fileca" }}
- name: PASSWORD
valueFrom:
secretKeyRef:
name: {{ .Values.server.secret }}
key: password
{{- end }}
{{- else if eq .Values.server.args.certificateAuthority "fileca" }}
env:
- name: PASSWORD
valueFrom:
secretKeyRef:
name: {{ .Values.server.secret }}
key: password
{{- end }}
volumeMounts:
- name: fulcio-config
mountPath: /etc/fulcio-config
Expand Down
19 changes: 19 additions & 0 deletions charts/fulcio/values.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,15 @@
"examples": [
"https://ct.example.com/"
]
},
"creds": {
"type": "string",
"default": "",
"title": "Google Cloud Web Identity JSON config / Service Account token key json",
"description": "If set, Fulcio will use these credentials to impersonate GCP service account from trusted external cloud providers",
"examples": []
}

},
"examples": [
{
Expand All @@ -272,6 +280,17 @@
}
]
},
"env": {
"type": "object",
"default": {},
"title": "The env Schema",
"properties": {},
"examples": [
{
"GOOGLE_APPLICATION_CREDENTIALS": "/etc/fulcio-config/cloud_credentials"
}
]
},
"serviceAccount": {
"type": "object",
"default": {},
Expand Down
5 changes: 5 additions & 0 deletions charts/fulcio/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,14 @@ server:
# crane digest gcr.io/projectsigstore/fulcio:v1.3.1
# -- v1.3.1
version: sha256:c920be2d367214562cda7d53d3af3529edf2dfd9326b24909ece450092b97b18
# env:
# # Valid values: path to workload identity config or service account key JSON file
# GOOGLE_APPLICATION_CREDENTIALS: /etc/fulcio-config/cloud_credentials
args:
port: 5555
grpcPort: 5554
# valid values: GCP workload identity config json for trusted external cloud providers
creds: ""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the name creds is somewhat ambitious

Copy link
Contributor Author

@saisatishkarra saisatishkarra May 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an expected naming suggestion? I used this to maintain the existing convention between TSA and Fulcio helm chart. How about cloud_credential_config?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am good with cloud_credential_config

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!!

# Valid values: googleca, pkcs11ca, aws-hsm-root-ca-path, fileca, kmsca
certificateAuthority: fileca
# kms_resource: gcpkms://....
Expand Down