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

Cosigned policies interpreted as regex by default #1871

Closed
znewman01 opened this issue May 12, 2022 · 12 comments · Fixed by #1956
Closed

Cosigned policies interpreted as regex by default #1871

znewman01 opened this issue May 12, 2022 · 12 comments · Fixed by #1956

Comments

@znewman01
Copy link
Contributor

Unrelated to this PR but it's a regex??? That's a surprise

Does this open up an attack where someone passes [email protected] and I go register mailqexample.com and can pass the check?

Originally posted by @znewman01 in #1869 (comment)

I don't think this is necessarily a problem except users shouldn't be surprised by it or do the wrong thing by default. So maybe: make it loudly warn, or make regex behavior configurable? Or support globs instead?

@znewman01
Copy link
Contributor Author

CC @vaikas

@hectorj2f
Copy link
Contributor

@znewman01 i am working on using globing and remove regex from the image pattern. There is already an issue

@znewman01
Copy link
Contributor Author

I see #1834...I'm not convinced that this is a dupe, it seems to be a different regex unless I'm misunderstanding something. Would that change make it so that an escaping step happens before we compile it as a regex in verify.go?

@hectorj2f
Copy link
Contributor

@znewman01 You are right! It is related to https://github.com/sigstore/cosign/blob/main/pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation.go#L200. For a moment, i believed it was related to #1834.

@hectorj2f
Copy link
Contributor

@znewman01 @vaikas Should the subject be a non-regexp instead ? Or you can see use cases here. If so, i am wondering if we should do the same for issuer.

@znewman01
Copy link
Contributor Author

My main concern is that users will accidentally enter something like an email (including .) which will then be validated in a surprising way.

I'm okay with users being able to specify a regex, and it seems useful. Maybe if it's opt-in? Or really obvious (users have to type r"[email protected]" or similar)? I'm not 100% familiar with the context.

@vaikas
Copy link
Contributor

vaikas commented May 13, 2022

Ok. How do folks feel if for v1beta1, we add something like a prefix, r: that indicates it's an regex? It's a bit goofy I suppose, but thoughts?

@znewman01
Copy link
Contributor Author

How do folks feel if for v1beta1, we add something like a prefix, r: that indicates it's an regex? It's a bit goofy I suppose, but thoughts?

I have one escaping concern long-term: r:[email protected] is a valid email. Though I suppose you could do r:r:foo@example\.com—still the same problem of "now r:[email protected] accidentally matches [email protected]. My preference is always going to be to fix this "at the type level".

But yeah something like that would be fine for v1beta1 :)

@hectorj2f
Copy link
Contributor

Yeah, let's solve it for v1beta1.

@vaikas
Copy link
Contributor

vaikas commented May 31, 2022

Just bringing this back. So, I think we have couple of options (I'm sure I missed some), but I think I'd rather tackle this hopefully in a more 'final' form sooner rather than later.
Here's few options:

  • Use a re: or some other prefix on the string
  • Create a 'flag' on the Identity that controls whether fields are interpreted as RE or not
  • Add SubjectRE and IssuerRE as parallel fields to Subject/Issuer so it's crystal clear that the user is wanting the RE

As I said I'm sure there are others, but I think I kind of like the last option for clarity and therefore no confusion on it.
@znewman01 @hectorj2f Thoughts?

And of course, the last option is that we do not allow any regexp at all, but I think it's useful. Maybe I'm wrong :)

@znewman01
Copy link
Contributor Author

Just bringing this back. So, I think we have couple of options (I'm sure I missed some), but I think I'd rather tackle this hopefully in a more 'final' form sooner rather than later. Here's few options:

  • Use a re: or some other prefix on the string
    I can live with this, but what if my email is "re:[email protected]"?
  • Create a 'flag' on the Identity that controls whether fields are interpreted as RE or not
    Kinda awkward
  • Add SubjectRE and IssuerRE as parallel fields to Subject/Issuer so it's crystal clear that the user is wanting the RE
    SGTM, this is my favorite. Just need to document what happens if both are provided

As I said I'm sure there are others, but I think I kind of like the last option for clarity and therefore no confusion on it. @znewman01 @hectorj2f Thoughts?

And of course, the last option is that we do not allow any regexp at all, but I think it's useful. Maybe I'm wrong :)

I agree it would be useful!

@hectorj2f
Copy link
Contributor

I definitely agree it is useful, I am just concerned about the prefix re: but I don't have a better proposal.

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 a pull request may close this issue.

3 participants