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

charts/tuf/templates/deployment.yaml

charts/tuf/templates/ingress.yaml

charts/tuf/templates/_helpers.tpl

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

haydentherapper · 2024-10-02T23:54:36Z

@cpanato does this look good to you?

cpanato

thanks

and sorry for the delay

cpanato · 2024-10-07T09:22:54Z

but need to update the docs, can you please take a look?

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

vipulagarwal · 2024-10-07T09:34:25Z

but need to update the docs, can you please take a look?

@cpanato the README was updated
is there anything else missing? would you like the description field to be populated?

cvegagimenez · 2024-10-07T10:42:19Z

@cpanato , the tests are failing but I am not able to find the issue. Could you help here?

{"level":"fatal","ts":1728294947.0237236,"logger":"fallback","caller":"server/main.go:247","msg":"failed to parse /var/run/tuf-secrets/tsa.certchain.pem: unmarshaling certificates: error during PEM decoding","stacktrace":"main.main\n\tgithub.com/sigstore/scaffolding/cmd/tuf/server/main.go:247\nruntime.main\n\truntime/proc.go:272"}

vipulagarwal · 2024-10-07T10:54:28Z

charts/tuf/templates/deployment.yaml

    {{- if .Values.imagePullSecrets }}
      imagePullSecrets:
 {{ toYaml .Values.imagePullSecrets | indent 8 }}
    {{- end }}
-    {{- if .Values.deployment.nodeSelector }}


why are we removing the nodeSelector, tolerations and affinity features for tuf deployment?

Completely right. Already fixed. Thanks!

vipulagarwal · 2024-10-07T11:11:38Z

@cpanato , the tests are failing but I am not able to find the issue. Could you help here?

{"level":"fatal","ts":1728294947.0237236,"logger":"fallback","caller":"server/main.go:247","msg":"failed to parse /var/run/tuf-secrets/tsa.certchain.pem: unmarshaling certificates: error during PEM decoding","stacktrace":"main.main\n\tgithub.com/sigstore/scaffolding/cmd/tuf/server/main.go:247\nruntime.main\n\truntime/proc.go:272"}

Looking at previous test runs, we always had this error but it was somehow still passing the chart installation (incorrect behaviour). The error happens because by default all the secrets are enabled and scaffolding tuf server expects a valid certificate for tsa certificate chain.

The test failing now is the correct behavior, presumable a result of latest scaffolding tuf server image. To fix the actual problem, we could set create: true for TSA secret but this enables it by default for everyone using the default values.

Edit: Or update scaffolding code to ignore empty tsa.certchain.pem file

charts/tuf/templates/deployment.yaml

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

cpanato

thanks

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

charts/tuf/templates/deployment.yaml Outdated Show resolved Hide resolved

charts/tuf/templates/ingress.yaml Outdated Show resolved Hide resolved

charts/tuf/templates/_helpers.tpl Outdated Show resolved Hide resolved

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]>

cpanato previously approved these changes Oct 7, 2024

View reviewed changes

cpanato self-requested a review October 7, 2024 09:23

Helm Docs for TUF

71247bc

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

cvegagimenez dismissed cpanato’s stale review via 71247bc October 7, 2024 09:34

vipulagarwal reviewed Oct 7, 2024

View reviewed changes

cpanato reviewed Oct 8, 2024

View reviewed changes

charts/tuf/templates/deployment.yaml Outdated Show resolved Hide resolved

Restored NodeSelector, Tolerations and Affinity

88f2633

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

cvegagimenez force-pushed the tsa-secret-optional-for-tuf branch from c46255f to 88f2633 Compare October 8, 2024 11:55

cpanato approved these changes Oct 14, 2024

View reviewed changes

cpanato merged commit 95cbdec into sigstore:main Oct 14, 2024
3 checks passed

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

haydentherapper commented Oct 2, 2024

cpanato left a comment

cpanato commented Oct 7, 2024

vipulagarwal commented Oct 7, 2024

cvegagimenez commented Oct 7, 2024 •

edited

Loading

vipulagarwal Oct 7, 2024

cvegagimenez Oct 7, 2024

vipulagarwal commented Oct 7, 2024 •

edited

Loading

cpanato left a comment

Tsa secret optional for tuf #744

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

haydentherapper commented Oct 2, 2024

cpanato left a comment

Choose a reason for hiding this comment

cpanato commented Oct 7, 2024

vipulagarwal commented Oct 7, 2024

cvegagimenez commented Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vipulagarwal commented Oct 7, 2024 • edited Loading

cpanato left a comment

Choose a reason for hiding this comment

cvegagimenez commented Apr 22, 2024 •

edited

Loading

cvegagimenez commented Oct 7, 2024 •

edited

Loading

vipulagarwal commented Oct 7, 2024 •

edited

Loading