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.

Sign up for GitHub

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

Jump to bottom

Implement constant-time secret comparisons in CEL. #492

Merged
merged 1 commit into from
Mar 31, 2020
Merged

Implement constant-time secret comparisons in CEL. #492

merged 1 commit into from
Mar 31, 2020

Conversation

bigkevmcd
Copy link
Member

Changes

This adds new compareSecret overload for strings in CEL, allowing comparison of a value against a secret.

This addresses #486

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide for more details.

Release Notes

New CEL interceptor function `compareSecrets` for securely comparing strings to secrets in CEL expressions.

@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 20, 2020
@bigkevmcd bigkevmcd mentioned this pull request Mar 20, 2020
Closed
@bigkevmcd
Copy link
Member Author

/test pull-tekton-triggers-build-tests/

@bigkevmcd
Copy link
Member Author

/test pull-tekton-triggers-build-tests

dibyom
dibyom reviewed Mar 24, 2020
View reviewed changes
Copy link
Member

@dibyom dibyom left a comment

Choose a reason for hiding this comment

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

Can we add an example with the secretCompare?

docs/cel_expressions.md Outdated
The event-listener service account must have access to the secret.
</td>
<td>
<pre>header.canonical('X-Secret-Token').compareSecret('namespace', 'secret-name', 'key')</pre>
Copy link
Member

Choose a reason for hiding this comment

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

can the namespace be optional? In other interceptors, if one is not specified, it defaults to the EL's namespace.

Copy link
Member Author

Choose a reason for hiding this comment

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

I implemented it with variable args, if you provide 3, the first is the namespace, only 2 and it defaults to the eventlistener's namespace.

Copy link
Member

@dibyom dibyom Mar 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ahh ok, that sounds good.
Thoughts on reversing the function params (key, secretName, ns) vs (ns, name, key) since the ns one is optional?

@bigkevmcd bigkevmcd force-pushed the cel-secret-access branch 2 times, most recently from 6f9932c to 38f2e7b Compare March 26, 2020 14:54
@bigkevmcd
Copy link
Member Author

/test pull-tekton-triggers-build-tests

@dibyom
Copy link
Member

dibyom commented Mar 30, 2020

/approve

One minor thing about the optional namespace position in the function signature but other wise 👍

@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 30, 2020
@bigkevmcd
Implement constant-time secret comparisons in CEL.
0a85a35 
This adds new compareSecret overload for strings in CEL, allowing comparison of a value against a secret.
@dibyom
Copy link
Member

dibyom commented Mar 31, 2020

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 31, 2020
@tekton-robot tekton-robot merged commit 57c4adb into tektoncd:master Mar 31, 2020
@dibyom dibyom mentioned this pull request Mar 31, 2020
Closed
@bigkevmcd bigkevmcd deleted the cel-secret-access branch May 27, 2020 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dibyom dibyom dibyom left review comments

@bobcatfish bobcatfish Awaiting requested review from bobcatfish

@wlynch wlynch Awaiting requested review from wlynch

Assignees

@dibyom dibyom

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bigkevmcd @dibyom @tekton-robot @googlebot