Skip to content

Conversation

@yogisinha
Copy link
Contributor

Why the changes in this PR are needed?

parse and verify x509 certificates with additional options was requested

What are the changes in this PR?

buitins.go and crypto.go were modified

Notes to assist PR review:

This is a draft PR as requested by @ashutosh-narkar.
Please comment if the implementation looks right

Further comments:

Just to pass the existing tests I created a another method verifyX509CertificateChainWithOpts which is same as verifyX509CertificateChain except it takes additional verifyOptions struct. I can remove this method and add this extra verifyOptions to verifyX509CertificateChain and change the test cases later. verifyX509CertificateChainWithOpts is just temporary.

For an e.g. This method will be called as :

crypto.x509.parse_and_verify_certificates_with_options(
pem encoded cert data,
'{
"DNSName": "some_value", "CurrentTime": "value in millis epoch", 
"KeyUsages": ["KeyUsageServerAuth", "KeyUsageClientAuth", "KeyUsageCodeSigning"]}",
"MaxConstraintComparisons": "int value"
)'
)

Squashing all the commits for following issue:
Fix for the issue when OPA doesnot load tarball on cmd line as a bundle.

Fixes open-policy-agent#5879

Signed-off-by: Yogesh Sinha <[email protected]>
@netlify
Copy link

netlify bot commented Feb 11, 2024

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit 91e9007
🔍 Latest deploy log https://app.netlify.com/sites/openpolicyagent/deploys/65fbfc10ae698f0008217e9a
😎 Deploy Preview https://deploy-preview-6583--openpolicyagent.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@ashutosh-narkar
Copy link
Member

@yogisinha thanks for the contribution. Can you please resolve the conflicts in the PR?

@yogisinha
Copy link
Contributor Author

I resolved the conflicts.

Copy link
Contributor

@charlieegan3 charlieegan3 left a comment

Choose a reason for hiding this comment

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

Hi @yogisinha, thanks for this PR. It's a great start and having these options available will be handy.

In addition to my comments below, I think it'd be good to have some test cases for the new function. You can see some examples here to get you started https://github.com/open-policy-agent/opa/blob/cfb03ca495111cbcce98acce93bf96b8433f4f35/topdown/crypto_test.go#L144-L143

Thanks again and sorry to take a while to get back to you. that's on me.

ast/builtins.go Outdated
Name: "crypto.x509.parse_and_verify_certificates_with_options",
Description: `Returns one or more certificates from the given string containing PEM
or base64 encoded DER certificates after verifying the supplied certificates form a complete
certificate chain back to a trusted root. Second argument is a option object to validate the
Copy link
Contributor

Choose a reason for hiding this comment

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

This line looks to be incomplete? the note about the options seems to be cut off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @charlieegan3 ,
Is there any way to see the documentation of functions on browser, serving the content from my local opa. I want to see how it looks on browser under "Built in functions" page for my new function description.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, the best way to check is:

make generate # update the top level generated builtin_metadata.json file
cd docs/
make dev-generate
cd website
hugo server --contentDir=generated/

Then visit: http://localhost:1313/docs/edge/policy-reference/#built-in-functions

Hope that helps!

Copy link
Contributor Author

@yogisinha yogisinha Feb 21, 2024

Choose a reason for hiding this comment

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

Hi,
its giving me following error on last step. I didn't have hugo so I installed it by sudo apt install hugo. I will commit the newly generated json files though.

~/opa/docs/website$ hugo server --contentDir=generated/

Building sites … ERROR 2024/02/21 11:55:13 render of "page" failed: "/home/yogi/opa/docs/website/layouts/docs/ecosystem-single.html.html:20:17": execute of template failed: template: docs/ecosystem-single.html.html:20:17: executing "content" at <path>: can't evaluate field BaseName in type interface {}
ERROR 2024/02/21 11:55:13 render of "page" failed: "/home/yogi/opa/docs/website/layouts/docs/ecosystem-single.html.html:20:17": execute of template failed: template: docs/ecosystem-single.html.html:20:17: executing "content" at <path>: can't evaluate field BaseName in type interface {}
ERROR 2024/02/21 11:55:13 render of "page" failed: "/home/yogi/opa/docs/website/layouts/docs/ecosystem-single.html.html:20:17": execute of template failed: template: docs/ecosystem-single.html.html:20:17: executing "content" at <path>: can't evaluate field BaseName in type interface {}
ERROR 2024/02/21 11:55:13 render of "page" failed: "/home/yogi/opa/docs/website/layouts/docs/ecosystem-single.html.html:20:17": execute of template failed: template: docs/ecosystem-single.html.html:20:17: executing "content" at <path>: can't evaluate field BaseName in type interface {}
Built in 1659 ms
Error: Error building site: failed to render pages: render of "page" failed: "/home/yogi/opa/docs/website/layouts/docs/ecosystem-single.html.html:20:17": execute of template failed: template: docs/ecosystem-single.html.html:20:17: executing "content" at <path>: can't evaluate field BaseName in type interface {}

Copy link
Contributor

Choose a reason for hiding this comment

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

ast/builtins.go Outdated
types.NewSet(types.A),
),
)),
).Description("object containing a set or array of neighboring vertices"),
Copy link
Contributor

Choose a reason for hiding this comment

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

neighboring vertices

What is this meant to refer to. I thought the options were meant to be the key usages?

return iter(valid)
}

type verifyOptions struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

question: is there a reason we can't use the golang type? https://pkg.go.dev/crypto/x509#VerifyOptions

return iter(valid)
}

func extractVerifyOpts(options ast.Object) (verifyOpt verifyOptions, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that it would be nice to have some tests for this function that map the Rego options to the golang struct (https://pkg.go.dev/crypto/x509#VerifyOptions)

return chains[0], nil
}

func verifyX509CertificateChainWithOpts(certs []*x509.Certificate, vo verifyOptions) ([]*x509.Certificate, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that rather than making a new function here we can extend verifyX509CertificateChain to support the passing of an existing options struct that is extended if present or initialised fresh if not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I plan to do that. because extending verifyX509CertificateChain was making some of the existing test cases fail so thats why I created the new function just to pass the tests as this was Draft PR. I will make the change and rest of your suggestions too. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 sounds like a good plan, thanks.

return verifyOpt, fmt.Errorf("CurrentTime could not be parsed to be a valid int64 number.")
}
} else {
return verifyOpt, fmt.Errorf("CurrentTime should be a number")
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, there are some linter errors for these messages since they start with uppercase letters. I think they could be adjusted to "field 'CurrentTime' should be a number" or similar to get around the issue.

@yogisinha yogisinha force-pushed the parse_and_verify_certificates_with_options branch from bf33692 to 35d2f85 Compare February 21, 2024 14:10
@yogisinha
Copy link
Contributor Author

@charlieegan3 ,
I made all the changes. Please take a look.

@charlieegan3
Copy link
Contributor

Hey, I have added a comment here #6583 (comment) regarding previewing the docs. I think you're also going to need to run make generate and commit the new builtins JSON too. Thanks!

@yogisinha
Copy link
Contributor Author

Hey, I have added a comment here #6583 (comment) regarding previewing the docs. I think you're also going to need to run make generate and commit the new builtins JSON too. Thanks!

Added the json files.

Copy link
Contributor

@charlieegan3 charlieegan3 left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test and the other edits based on my comments above. It's taking shape, just a few more minor ones.

ast/builtins.go Outdated
Description: `Returns one or more certificates from the given string containing PEM
or base64 encoded DER certificates after verifying the supplied certificates form a complete
certificate chain back to a trusted root. Second argument is a option object to pass
extra configs to verify the validity of certificates.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Second argument is a option object to pass extra configs to verify the validity of certificates."

I would change this to:

"A config option passed as the second argument can be used to configure the validation options used."

ast/builtins.go Outdated
extra configs to verify the validity of certificates.
The first certificate is treated as the root and the last is treated as the leaf,
with all others being treated as intermediates.` +
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
with all others being treated as intermediates.` +
with all others being treated as intermediates.
` +
Screenshot 2024-02-21 at 18 11 56

ast/builtins.go Outdated
"`\"MaxConstraintComparisons\" : 5,`" +
"`}`" +
"Then this function call will look like: `crypto.x509.parse_and_verify_certificates_with_options(<cert base64 encoded string>, options)`" +
"This option's field has same definitions as fields in [x509.VerifyOptions struct](https://pkg.go.dev/crypto/x509#VerifyOptions)" +
Copy link
Contributor

Choose a reason for hiding this comment

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

Screen.Recording.2024-02-21.at.18.13.21.mov

I think that we need to make some of these lines less long, as it's pushing out the rest of the table at the moment. Some strategic line breaks should be 👌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I realized that too and was already working on that

ast/builtins.go Outdated
"`\"MaxConstraintComparisons\" : 5,`" +
"`}`" +
"Then this function call will look like: `crypto.x509.parse_and_verify_certificates_with_options(<cert base64 encoded string>, options)`" +
"This option's field has same definitions as fields in [x509.VerifyOptions struct](https://pkg.go.dev/crypto/x509#VerifyOptions)" +
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"This option's field has same definitions as fields in [x509.VerifyOptions struct](https://pkg.go.dev/crypto/x509#VerifyOptions)" +
"This options object has the same fields as [x509.VerifyOptions](https://pkg.go.dev/crypto/x509#VerifyOptions)" +

ast/builtins.go Outdated
"`}`" +
"Then this function call will look like: `crypto.x509.parse_and_verify_certificates_with_options(<cert base64 encoded string>, options)`" +
"This option's field has same definitions as fields in [x509.VerifyOptions struct](https://pkg.go.dev/crypto/x509#VerifyOptions)" +
"`CurrentTime` is nanoseconds since epoch" +
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"`CurrentTime` is nanoseconds since epoch" +
"`CurrentTime` is the number of nanoseconds since the Unix Epoch as a number" +


for _, key := range options.Keys() {
switch key.String() {
case "\"DNSName\"":
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be nicer to clean the quote from the key at the top of the loop iteration, this might make it look a little more tidy in the cases.

})

} else {
return verifyOpt, fmt.Errorf("'KeyUsages' should be a set")
Copy link
Contributor

Choose a reason for hiding this comment

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

Might we want to support the case where the user supplies an array?

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 decided on Set because it automatically dedups the Key usage entry if user has supplied same entry more than once. Array would not do that. LMK what you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was just imagining that a user might provide an array by mistake, if it were coming from input or data loaded from JSON this is quite likely. I agree that a Set is what we'd like to receive in an ideal case, but I think there's merit in covering an array as input too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense. I would change that.

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 think having dup in array is ok. its not a issue. I looked into Go code and passing list with dup key usages should be fine. just extra loop executions in this part of code: https://cs.opensource.google/go/go/+/refs/tags/go1.22.0:src/crypto/x509/verify.go;l=1166

Copy link
Contributor Author

@yogisinha yogisinha Feb 22, 2024

Choose a reason for hiding this comment

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

I added the Array type for KeyUsages.

}{
{
jsonOption: ast.MustParseTerm(`{"DNSName": 1}`),
expectErr: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to know what the error was in such cases, perhaps checking a string or something in the err.Error()

@charlieegan3
Copy link
Contributor

Hey @yogisinha, are you able to take a look at the conflicts with main again?

@yogisinha yogisinha closed this Feb 27, 2024
@yogisinha yogisinha reopened this Feb 27, 2024
@yogisinha
Copy link
Contributor Author

Are you talking about this message "This branch is out-of-date with the base branch" . This was not coming before. What should I do ? Should I do "Update with merge commit" ?

@charlieegan3
Copy link
Contributor

Hey, yeah best would be to run:

git fetch origin
git rebase origin/main
# address any conflicts and make sure the tests all pass etc
git rebase --continue
git push -f

Copy link
Member

@ashutosh-narkar ashutosh-narkar left a comment

Choose a reason for hiding this comment

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

@yogisinha thanks for working on this. It would be helpful to add a unit test that actually tests the new builtin itself in addition to what you already have. Thanks.

@yogisinha
Copy link
Contributor Author

Added the test case.

Copy link
Member

@ashutosh-narkar ashutosh-narkar left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test cases and addressing other comments. Please squash your commits and we can get this in.

Copy link
Contributor

@charlieegan3 charlieegan3 left a comment

Choose a reason for hiding this comment

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

Hi @yogisinha, thanks for all the work on this. I think the code is looking great now.

My only final comment is that the docs are still pushing out the table. This built-in table is a bit of a pain to work with... but I'd like to keep it without a h-scroll bar if possible. Perhaps the options can be a bulleted list?

Screen.Recording.2024-03-12.at.09.15.54.mov

The video compares main and this branch in different tabs

@yogisinha
Copy link
Contributor Author

@charlieegan3 ,
I think it looks good now. I had to make several commits incrementally to see the behavior of deploy preview.

if err != nil {
return verifyOpt, err
}
k = k.(string)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since ast.JSON returns an interface{}, I think we need to make this a check too.

E.g. key, ok := k.(string)

and then if !ok { ... // return err etc }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But giving keys that are not string type will be caught by rego type checker. I tried that and it threw the error and won't even call the function. Shall I still add it ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh I see, hmm... perhaps we can just continue in the loop if the key is not a string?

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

Copy link
Contributor

@charlieegan3 charlieegan3 left a comment

Choose a reason for hiding this comment

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

This looks good to me now, I can still see that the table is overflowing but there are other overflowing tables on the page (net functions) so I think we need to do some more work on the site's styles there.

I have one final comment, then we can rebase and get this in.

yogisinha and others added 3 commits March 18, 2024 11:48
Fixes open-policy-agent#5882

Signed-off-by: Yogesh Sinha <[email protected]>
This adds the new builtin to the capabilities.json and
builtin_metadata.json

Signed-off-by: Charlie Egan <[email protected]>
@charlieegan3
Copy link
Contributor

Hey, I think we are nearly there now @yogisinha, I have updated the generated code in 91e9007 but can you rebase this from origin/main so that we can get this in. I'm at KubeCon at the moment so have limited time to help, but will reply when I get a chance.

Thanks for your work here, much appreciated!

@yogisinha
Copy link
Contributor Author

Hi @charlieegan3 ,
I created a new PR . I was getting lot of conflicts when I was trying to rebase, don't know why. In that new PR, everything is in single commit.

New PR: #6643

@ashutosh-narkar
Copy link
Member

Closing this in favor of #6643.

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.

3 participants