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.

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

Tsa secret optional for tuf #744

Merged
merged 16 commits into from
Oct 14, 2024

Conversation

cvegagimenez
Copy link
Contributor

@cvegagimenez cvegagimenez commented Apr 22, 2024

Description of the change

Make the TSA secret reference optional for TUF chart.

Existing or Associated Issue(s)

#735

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

@@ -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` | |
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sabre1041 Any news on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@sabre1041 sabre1041 left a comment

Choose a reason for hiding this comment

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

@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

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

@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.

@cvegagimenez cvegagimenez force-pushed the tsa-secret-optional-for-tuf branch from 6ce1d8b to 91c9839 Compare September 30, 2024 16:47
sabre1041
sabre1041 previously approved these changes Sep 30, 2024
Copy link
Contributor

@sabre1041 sabre1041 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

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 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
Signed-off-by: Carlos Vega <[email protected]>
Signed-off-by: Carlos Vega <[email protected]>
@cvegagimenez cvegagimenez force-pushed the tsa-secret-optional-for-tuf branch from 029239c to e8c7d53 Compare October 1, 2024 08:08
@haydentherapper
Copy link
Contributor

@cpanato does this look good to you?

cpanato
cpanato previously approved these changes Oct 7, 2024
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

thanks

and sorry for the delay

@cpanato
Copy link
Member

cpanato commented Oct 7, 2024

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

@cpanato cpanato self-requested a review October 7, 2024 09:23
Signed-off-by: Carlos Vega <[email protected]>
@vipulagarwal
Copy link
Contributor

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
Copy link
Contributor Author

cvegagimenez commented Oct 7, 2024

@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"}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completely right. Already fixed. Thanks!

@vipulagarwal
Copy link
Contributor

vipulagarwal commented Oct 7, 2024

@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

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

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

thanks

@cpanato cpanato merged commit 95cbdec into sigstore:main Oct 14, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants