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

Fixing CA Path Flag to be used and adding policy timestamp server flag #353

Merged
merged 30 commits into from May 16, 2024

Conversation

ChaosInTheCRD
Copy link
Collaborator

@ChaosInTheCRD ChaosInTheCRD commented Jan 11, 2024

This PR contains a fix for the noticed bug that was discovered in that vo.PolicyCAPaths was not used in the Verify function. Note that this change working correctly is dependent on changes to https://github.com/in-toto/go-witness.

The other half of this PR focuses on allowing users to sign their policy a timestamp authority, and if they have done so, allow them to supply the CA certificate of the timestamp authority server for use in the policy verification flow. Once again this change is dependent on changes to go-witness.

This PR depends on in-toto/go-witness#124

specified for policy check

Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
@jkjell
Copy link
Member

jkjell commented Jan 11, 2024

Can we add the same additional flags that Sigstore uses? https://docs.sigstore.dev/verifying/verify/#keyless-verification-using-openid-connect

--certificate-identity=name@example.com and --certificate-oidc-issuer=https://accounts.example.com

cmd/verify.go Outdated
@@ -56,23 +58,58 @@ const (
// todo: this logic should be broken out and moved to pkg/
Copy link
Member

Choose a reason for hiding this comment

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

We should address this todo as a part of this as well. The verification for a policy and an attestation should be the same I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah for sure. The only difference with policy is that there isn't going to be a policy document for verifying the policy... if that makes sense. So we ought to keep the number of ways that one can use to verify the policy signature small...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you think it makes sense to make all these changes in this PR?

Copy link
Member

Choose a reason for hiding this comment

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

only difference with policy is that there isn't going to be a policy document for verifying the policy

I think it would be the same as the step verification: https://github.com/in-toto/go-witness/blob/main/policy/policy.go#L230

Copy link
Member

Choose a reason for hiding this comment

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

We're essentially defining the functionary from the CLI

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah the problem though is that defining all the available parameters for the functionaries from the CLI could quickly become unwieldly imo. Consider this functionary:

        {
          "type": "root",
          "certConstraint": {
            "commonname": "*",
            "dnsnames": [
              "*"
            ],
            "emails": [
              "tpmeadows1@gmail.com"
            ],
            "organizations": [
              "*"
            ],
            "uris": [
              "*"
            ],
            "roots": [
              "wippywoo"
            ]
          }
        }

In this case, we would need to define:

  1. cert-constraints (commonName, DNSName, emails, organizations, uris)
  2. roots (this includes root CA cert, intermediates (which doesn't quite seem clear in the tools current form), and we might need to supply timestamp servers alongside that.

How we would present that in a flag form I am not quite sure on. To me I think that limiting support for the number of policy signing mechanisms might be the way forward? It also seems to be the case to me that TUF may be a solution to this. @kairoaraujo any thoughts?

Copy link
Member

@colek42 colek42 Jan 14, 2024

Choose a reason for hiding this comment

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

I agree that it is unwieldy, however I would normally expect the user to wrap this in a bash script, or maybe a "witness verify" github action. I also think we could provide some syntactic sugar for sigstore that would let us provide better UX for the common use case while being secure by default.

ex:

for enterprise use:
witness verify -p signedpolicy.json -k ca.pem -i int.pem --commonname="*" --dnsname="*" --email="tpmeadows1@gmail.com" --organization="*" --uri="*" --root="wippywoo" artifact.tar

for sigstore:
witness verify -p signedpolicy.json --use-sigstore --use-archivista --email="tpmeadows1@gmail.com" artifact.tar

We would also need to provide a TSA pem or add support for Rekor

@ChaosInTheCRD ChaosInTheCRD marked this pull request as ready for review January 29, 2024 16:07
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
@ChaosInTheCRD
Copy link
Collaborator Author

This PR is dependent on in-toto/go-witness#144

@mikhailswift
Copy link
Collaborator

#114's been merged, can we get this updated and ready for final review?

Signed-off-by: Tom Meadows <tom@tmlabs.co.uk>
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
@jkjell
Copy link
Member

jkjell commented Feb 20, 2024

@ChaosInTheCRD I've been thinking a bit about this and I think this might be a blocker for this PR. One of the critical things to verify on a Fulcio cert is the Issuer, which is an extension we don't support yet.

@ChaosInTheCRD
Copy link
Collaborator Author

@jkjell good that you noted these custom cert extensions and I certainly think we should follow up with work to support them. However, that work is relevant to https://github.com/in-toto/go-witness, and moreover this work specifically aims to bring policy signature verification logic up to scratch with what we are currently able to use to verify attestation signatures.

Provided you're happy, I suggest we merge this PR for now. We then follow up with subsequent PRs in https://github.com/in-toto/go-witness and then finally implement it here in https://github.com/in-toto/witness.

Note that (I probably sound like a broken record, but) we need to consider how these extra custom extensions might affect the total number of flags. I think as an MVP just adding them will be fine, but I think some consideration into how we could potentially consolidate them might be worthwhile.

@ChaosInTheCRD
Copy link
Collaborator Author

What I will say however, is if you think this is high priority enough, I can expedite this and we can try and bundle it into this PR.

Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
Copy link

netlify bot commented Feb 21, 2024

Deploy Preview for witness-project canceled.

Name Link
🔨 Latest commit 6e5265e
🔍 Latest deploy log https://app.netlify.com/sites/witness-project/deploys/6645f4245214d00008542978

@jkjell
Copy link
Member

jkjell commented May 2, 2024

Looks like we're missing a couple of test files: ./test/policy-signed.json and ./test/policy.json.

Signed-off-by: Tom Meadows <tom@tmlabs.co.uk>
@ChaosInTheCRD
Copy link
Collaborator Author

I need to loop back over this so we can get it ready for merge. This includes factoring in in-toto/go-witness#174

@jkjell jkjell marked this pull request as draft May 3, 2024 21:29
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
…o fixing-ca-path

Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
Signed-off-by: Tom Meadows <tom@tmlabs.co.uk>
@ChaosInTheCRD ChaosInTheCRD marked this pull request as ready for review May 13, 2024 17:56
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
colek42
colek42 previously approved these changes May 13, 2024
Signed-off-by: John Kjell <john@testifysec.com>
jkjell and others added 4 commits May 13, 2024 16:57
Signed-off-by: John Kjell <john@testifysec.com>
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
@jkjell jkjell merged commit b951db3 into in-toto:main May 16, 2024
17 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.

None yet

4 participants