Tsa secret optional for tuf #744

cvegagimenez · 2024-04-22T11:26:38Z

Description of the change

Make the TSA secret reference optional for TUF chart.

Existing or Associated Issue(s)

Additional Information

Checklist

Chart version bumped in Chart.yaml according to semver. Where applicable, update and bump the versions in any associated umbrella chart
Variables are documented in the values.yaml and added to the README.md. The helm-docs utility can be used to generate the necessary content. Use helm-docs --dry-run to preview the content.
JSON Schema generated.
List tests pass for Chart using the Chart Testing tool and the ct lint command.


------------------------------------------------------------------------------------------------------------------------
 Charts to be processed:
------------------------------------------------------------------------------------------------------------------------
 tuf => (version: "0.1.13", path: "charts/tuf")
------------------------------------------------------------------------------------------------------------------------

"sigstore" already exists with the same configuration, skipping
Linting chart "tuf => (version: \"0.1.13\", path: \"charts/tuf\")"
Checking chart "tuf => (version: \"0.1.13\", path: \"charts/tuf\")" for a version bump...
Old chart version: 0.1.12
New chart version: 0.1.13
Chart version ok.
Validating ~/Workspace/helm-charts/charts/tuf/Chart.yaml...
Validation success! 👍

Linting chart with values file "charts/tuf/ci/ci-values.yaml"...

==> Linting charts/tuf
[INFO] Chart.yaml: icon is recommended

1 chart(s) linted, 0 chart(s) failed

------------------------------------------------------------------------------------------------------------------------
 ✔︎ tuf => (version: "0.1.13", path: "charts/tuf")
------------------------------------------------------------------------------------------------------------------------
All charts linted successfully```

Signed-off-by: Carlos Vega <[email protected]>

sabre1041 · 2024-05-09T05:09:57Z

charts/tuf/README.md

@@ -54,6 +54,7 @@ A framework for securing software update systems - the scaffolding implementatio
 | secrets.rekor.name | string | `"rekor-public-key"` |  |
 | secrets.rekor.path | string | `"rekor.pub"` |  |
 | secrets.tsa.create | bool | `false` |  |
+| secrets.tsa.existingSecret | bool | `false` |  |


Why not make this enabled and set as true. If secrets.tsa.create is true, a new secret will be created. Otherwise, secrets.tsa.name is the name of the existing secret

I implemented the changes. I thought it would be better to set it as enabled as false by default since the Charts are independent, but I could change it if you consider the other way.

I like this approach a lot! It would be great if we can implement the same pattern across all of the secrets. Though that probably requires a separate PR to avoid encompassing too much into this issue. Unless you would want to rename the PR for that scope

As the TSA case, I set the default value to false to be independent.

@sabre1041 Any news on this?

@cvegagimenez syntactically this does work. However, in practice, for tuf to run properly, it will need at least one source of content (a secret) in order to start properly. Should we enforce that at least one secret is provided?

Also, would you be able to resolve the conflict in the README.md file?

Done. I also added the same checks for the other TUF objects.

Signed-off-by: Carlos Vega <[email protected]>

sabre1041

@cvegagimenez instead of omitting content when no secrets have been provided, an error should be thrown to ensure that the user provides at least one secret

Signed-off-by: Carlos Vega <[email protected]>

sabre1041 · 2024-09-30T05:31:08Z

@cvegagimenez This looks good., However, while thinking it through in practice enabled should by default be true as it aligns with the current functionality of the chart. By setting to false, it would be a breaking change for anyone currently leveraging it.

The goal of this PR is to provide a way to opt out of providing secrets a, but in practice, this has now introduced the functionality where you have to opt in to achieve the current functionality. A simple swap of the default values as we should be good to integrate this change.

Signed-off-by: Carlos Vega <[email protected]>

sabre1041

LGTM

cpanato

thanks

need to check the helm docs job i think need to update the readme as well and some small nits

cpanato · 2024-10-01T07:27:24Z

charts/tuf/templates/deployment.yaml

-      affinity:
-{{ toYaml .Values.deployment.affinity | indent 8 }}
-    {{- end }}
+    {{- end }}


nit: new line

cpanato · 2024-10-01T07:27:27Z

charts/tuf/templates/ingress.yaml

@@ -33,4 +33,4 @@ spec:
      secretName: {{ .secretName }}
    {{- end }}
 {{- end -}}
-{{- end -}}
+{{- end -}}


nit: new line

cpanato · 2024-10-01T07:27:34Z

charts/tuf/templates/_helpers.tpl

+        path: {{ .Values.secrets.tsa.path }}
+  {{- end }}
+{{- end }}


nit: new line

Signed-off-by: Carlos Vega <[email protected]>

cvegagimenez added 2 commits April 22, 2024 12:02

Make optional the secret TSA secret reference for TUF chart

e413ede

Signed-off-by: Carlos Vega <[email protected]>

Bump TUF chart version to 0.1.13

d156f58

Signed-off-by: Carlos Vega <[email protected]>

sabre1041 reviewed May 9, 2024

View reviewed changes

cvegagimenez and others added 6 commits May 9, 2024 15:49

Renamed Values.secrets.tsa.existingSecret to Values.secrets.tsa.enabled

106dbea

Signed-off-by: Carlos Vega <[email protected]>

Regenerated docs

f2346b7

Signed-off-by: Carlos Vega <[email protected]>

Added enabled value for secrets defined in tuf

e54a0b3

Signed-off-by: Carlos Vega <[email protected]>

Merge branch 'main' into tsa-secret-optional-for-tuf

55c4421

Signed-off-by: Carlos Vega <[email protected]>

Merge branch 'main' into tsa-secret-optional-for-tuf

89630d3

Signed-off-by: Carlos Vega <[email protected]>

Avoid creating Tuf objects if there is no secrets

0bd3586

Signed-off-by: Carlos Vega <[email protected]>

sabre1041 requested changes Sep 26, 2024

View reviewed changes

vipulagarwal mentioned this pull request Sep 26, 2024

TSA secret always required for TUF deployment #735

Open

Added validation to ensure at least one secret is provided to TUF

46a4a82

Signed-off-by: Carlos Vega <[email protected]>

cvegagimenez force-pushed the tsa-secret-optional-for-tuf branch from 69e91b6 to 46a4a82 Compare September 29, 2024 18:09

cvegagimenez added 2 commits September 30, 2024 07:50

Enabled secret as default for TUF

db6ba2b

Signed-off-by: Carlos Vega <[email protected]>

Bump version to 0.1.19

91c9839

Signed-off-by: Carlos Vega <[email protected]>

cvegagimenez force-pushed the tsa-secret-optional-for-tuf branch from 6ce1d8b to 91c9839 Compare September 30, 2024 16:47

sabre1041 previously approved these changes Sep 30, 2024

View reviewed changes

cpanato requested changes Oct 1, 2024

View reviewed changes

Helm Docs for TUF

c716603

Signed-off-by: Carlos Vega <[email protected]>

cvegagimenez dismissed sabre1041’s stale review via c716603 October 1, 2024 07:33

cvegagimenez force-pushed the tsa-secret-optional-for-tuf branch from 23368d7 to c716603 Compare October 1, 2024 07:33

Added new lines

e8c7d53

Signed-off-by: Carlos Vega <[email protected]>

cvegagimenez force-pushed the tsa-secret-optional-for-tuf branch from 029239c to e8c7d53 Compare October 1, 2024 08:08

Merge branch 'main' into tsa-secret-optional-for-tuf

62f5ded

Signed-off-by: Carlos Vega <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tsa secret optional for tuf #744

Tsa secret optional for tuf #744

cvegagimenez commented Apr 22, 2024 •

edited

Loading

sabre1041 May 9, 2024

cvegagimenez May 9, 2024

sabre1041 May 9, 2024

cvegagimenez May 10, 2024

cvegagimenez Jul 12, 2024

sabre1041 Sep 26, 2024

cvegagimenez Sep 26, 2024

sabre1041 left a comment

sabre1041 commented Sep 30, 2024

sabre1041 left a comment

cpanato left a comment

cpanato Oct 1, 2024

cpanato Oct 1, 2024

cpanato Oct 1, 2024

Tsa secret optional for tuf #744

Are you sure you want to change the base?

Tsa secret optional for tuf #744

Conversation

cvegagimenez commented Apr 22, 2024 • edited Loading

Description of the change

Existing or Associated Issue(s)

Additional Information

Checklist

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

sabre1041 left a comment

Choose a reason for hiding this comment

sabre1041 commented Sep 30, 2024

sabre1041 left a comment

Choose a reason for hiding this comment

cpanato 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

cvegagimenez commented Apr 22, 2024 •

edited

Loading