Adds strict match annotation + updated tests/docs #8582

chrisshino · 2022-05-13T13:47:39Z

What this PR does / why we need it:

This officially closes: #8330, this patch is needed to satisfy some of the feedbacks received on the original branch that was already merged in.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation only

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have added tests to cover my changes.
All new and existing tests passed.

k8s-ci-robot · 2022-05-13T13:47:48Z

Hi @chrisshino. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

chrisshino · 2022-05-13T13:52:47Z

#8434 In this pull request James mentions a few things that would improve this annotation and I implemented them.

Pulling out the CN from the client certificates DN (this has been done, meaning no longer do we have to add "CN=" before the regex or string to match.
Having an option to choose between either a strict match or regex match (these have both been implemented here)

rikatz · 2022-05-14T13:05:24Z

docs/user-guide/nginx-configuration/annotations.md

@@ -28,7 +28,8 @@ You can add these Kubernetes annotations to specific Ingress objects to customiz
 |[nginx.ingress.kubernetes.io/auth-tls-verify-client](#client-certificate-authentication)|string|
 |[nginx.ingress.kubernetes.io/auth-tls-error-page](#client-certificate-authentication)|string|
 |[nginx.ingress.kubernetes.io/auth-tls-pass-certificate-to-upstream](#client-certificate-authentication)|"true" or "false"|
-|[nginx.ingress.kubernetes.io/auth-tls-match-cn](#client-certificate-authentication)|string|


let's not deprecate an annotation. Instead, the old behavior should be called this way, and the new one can have a new name

makes sense! I will keep the original one here as the regex version, and then only add a new one for the strict mode!

rikatz · 2022-05-14T13:05:39Z

go.mod

@@ -15,7 +15,7 @@ require (
 	github.com/mitchellh/mapstructure v1.5.0
 	github.com/moul/pb v0.0.0-20180404114147-54bdd96e6a52
 	github.com/ncabatoff/process-exporter v0.7.10
-	github.com/onsi/ginkgo v1.16.5
+	github.com/onsi/ginkgo v1.16.4


why is this being downgraded?

these must have happened after I did a go mod tidy, im not sure why Ill make sure they are the same as before!

rikatz · 2022-05-14T13:05:51Z

go.mod

@@ -40,7 +40,7 @@ require (
 	k8s.io/component-base v0.23.6
 	k8s.io/klog/v2 v2.60.1
 	pault.ag/go/sniff v0.0.0-20200207005214-cf7e4d167732
-	sigs.k8s.io/controller-runtime v0.11.2
+	sigs.k8s.io/controller-runtime v0.10.3


why is this being downgraded?

rikatz · 2022-05-16T13:12:54Z

internal/ingress/annotations/authtls/main_test.go

+	if u.ExactMatchCN != "hello-app" {
+		t.Errorf("expected %v but got %v", "hello-app", u.ExactMatchCN)
+	}
+	if match, _ := regexp.MatchString(u.RegexMatchCN, "hello"); !match {


please don't ignore the error here

rikatz · 2022-05-16T13:13:53Z

internal/ingress/annotations/authtls/main.go

-	config.MatchCN, err = parser.GetStringAnnotation("auth-tls-match-cn", ing)
-	if err != nil || !commonNameRegex.MatchString(config.MatchCN) {
-		config.MatchCN = ""
+	config.ExactMatchCN, err = parser.GetStringAnnotation("auth-tls-exact-match-cn", ing)


Is there any priority here? Like exact > regex? If so, we should early exit if there's an exact match, otherwise try the regex.

since they are essentially 2 different annotations there isn't necessarily a priority.

rikatz · 2022-05-16T13:16:03Z

internal/ingress/annotations/authtls/main.go

+		config.ExactMatchCN = ""
+	}
+
+	config.RegexMatchCN, err = parser.GetStringAnnotation("auth-tls-regex-match-cn", ing)


I want actually that we validate if this is a valid regex per NGINX and not just add it to nginx.conf.

Based on the feedbacks, do we have a common pattern of required strings, like " REGEX = domain.*" or something like that, in a way we can sanitize/remove possible dangerous characters?

If so, let's do this (in both fields actually, we should validate what character set is allowed here)

hmm yea we can do this, I think it will always something like domain.*, but for example say the person wanted to use a regex match and the CN was = "my.example.com" and their regex only contained "example", would it fail because it doesn't have a dot after?

Or what if the domain contains numbers? Any suggestions on what a good pattern could look like here? probably something that contains letters, numbers or dots, but no other characters (for example, brackets "<", ">" ?

rikatz

@chrisshino thanks for the PR!

I've left some comments here, happy to discuss about it.

Thanks

chrisshino · 2022-05-16T15:08:16Z

@rikatz thank you for the thorough review 🙏 , I believe this should address your comments

chrisshino · 2022-05-16T16:40:10Z

hmm so whenever the e2e test runs, the go mod is switching those 2 dependency that you called out above to the lower versions?

Then it is failing, not sure why?

iamNoah1

/triage accepted
/priority backlog

iamNoah1 · 2022-06-10T12:53:29Z

/kind bug

k8s-triage-robot · 2022-11-25T13:23:03Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

rikatz · 2023-01-08T22:46:20Z

@chrisshino 👋 long time no see, sorry!
So I've left a comment on fixing the exactMatch vs Match, remember? if you implement this, I will gladly approve and merge the PR :)

Thank you!!!

rikatz · 2023-01-08T22:46:31Z

/lifecycle active

linux-foundation-easycla · 2023-01-14T18:38:13Z

The committers listed above are authorized under a signed CLA.

✅ login: chrisshino / name: Chris Shino (106af0f, 2c0f3f7)

k8s-ci-robot · 2023-01-14T18:38:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chrisshino
Once this PR has been reviewed and has the lgtm label, please ask for approval from rikatz by writing /assign @rikatz in a comment. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

chrisshino · 2023-01-14T18:49:37Z

/assign @rikatz

chrisshino · 2023-01-14T18:51:14Z

@rikatz okay so it looks like in all the back and forth I had made a git mistake and pushed the wrong thing, I have since updated that and pushed all my actual fixes that addressed your issues!

For some reason my CLA is still failing because of a commit message? I dont want to mess with git anymore than I have already lol do you know a good way of fixing this? Then this PR should be good to go!

Thanks again and sorry it took so long

rikatz · 2023-01-16T03:00:25Z

ah yeah, the joys of git :D been there, had the same problem as you, so let's map them:

The commit error message is because on one of your commits you added my handler (I saw something like @rikatz comments etc)
If by some reason you changed your email during commits, CLA gets kind of crazy. You can even add your other emails on your github profile, or rebase here

To rebase:

Make a backup of your git folder (in the end, if all goes wrong you can just copy over it! but really, do a backup)
git remote add upstream git@github.com:kubernetes/ingress-nginx.git
git rebase upstream/main
- If there are some conflicts...welll..We can zoom some day so we can look into conflicts and cry together
If no conflicts, do a "git log". Your commits should be the last ones, over something like upstream/main
Take the hash of were upstream/main is (like b02130912321c0aa)
git rebase -i the_hash_above
A vim/nano will open
First line you keep as is
The rest of lines where is written "pick" you replace for the char "f"
If you do git log again, now all your commits turned into one
git push origin branch-name -f

Helps? :D Otherwise, we can see what to do.

chrisshino · 2023-01-21T18:14:56Z

@rikatz wow thanks for the step by step instructions!!

Looks like everything should be good now, its just running through CI stuff but I have the correct tags now

k8s-ci-robot · 2023-01-31T06:35:59Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sherifkayad · 2023-08-03T08:20:42Z

@chrisshino @rikatz
I would like to report a bug in the previous implementation #8434 (I am currently using controller version 1.8.1) and that I would kindly like to see addressed by this PR. Referring to the comment here #9250 (comment)

Long story short: the combination of using ACME (e.g. letsencrypt) to issue a server cert + using client auth certs on ingress-nginx together with nginx.ingress.kubernetes.io/auth-tls-match-cn: "CN=my-cn" would result in a problem that the ACME cert can not be obtained. The reason here is that the CN check also runs for http which is wrong in my view and must only be performed when the scheme is https. As a workaround / mitigation I stopped using the nginx.ingress.kubernetes.io/auth-tls-match-cn annotation and instead I have a server snippet annotation as follows:

nginx.ingress.kubernetes.io/server-snippet: |
      set $CERT_CHECK "";
      if ($scheme = https) {
        set $CERT_CHECK "HTTPS";
      }
      if ($ssl_client_s_dn !~ "CN=my-cn") {
        set $CERT_CHECK "${CERT_CHECK}_NOCNMATCH";
      }
      if ($CERT_CHECK = HTTPS_NOCNMATCH) {
        return 403 "client certificate unauthorized: $ssl_client_s_dn\n";
      }

In my view, the check using the auth-tls-match-cn should be enhanced to be something like the logic I pasted above.

k8s-ci-robot added needs-priority size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 13, 2022

k8s-ci-robot requested a review from ElvinEfendi May 13, 2022 13:47

k8s-ci-robot added the area/docs label May 13, 2022

k8s-ci-robot requested a review from tao12345666333 May 13, 2022 13:47

rikatz reviewed May 14, 2022

View reviewed changes

rikatz reviewed May 16, 2022

View reviewed changes

rikatz requested changes May 16, 2022

View reviewed changes

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 16, 2022

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 17, 2022

iamNoah1 reviewed Jun 10, 2022

View reviewed changes

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority labels Jun 10, 2022

k8s-ci-robot assigned rikatz Oct 26, 2022

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 25, 2022

k8s-ci-robot added lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels Jan 8, 2023

chrisshino force-pushed the auth-tls-match-cn-patch branch from 520d69b to c88afeb Compare January 14, 2023 18:38

k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jan 14, 2023

k8s-ci-robot removed the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 14, 2023

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jan 14, 2023

chrisshino added 2 commits January 21, 2023 13:04

Adds strict match annotation + updated tests/docs

106af0f

keep go mod and go sum same as main branch

2c0f3f7

chrisshino force-pushed the auth-tls-match-cn-patch branch from c88afeb to 2c0f3f7 Compare January 21, 2023 18:12

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 31, 2023

sherifkayad mentioned this pull request Aug 3, 2023

The auth-tls-match-cn shouldn't enable for http #9250

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds strict match annotation + updated tests/docs #8582

Adds strict match annotation + updated tests/docs #8582

chrisshino commented May 13, 2022 •

edited

Loading

k8s-ci-robot commented May 13, 2022

chrisshino commented May 13, 2022

rikatz May 14, 2022

chrisshino May 16, 2022

rikatz May 14, 2022

chrisshino May 16, 2022

rikatz May 14, 2022

rikatz May 16, 2022

rikatz May 16, 2022

chrisshino May 16, 2022

rikatz May 16, 2022

chrisshino May 16, 2022 •

edited

Loading

rikatz left a comment

chrisshino commented May 16, 2022

chrisshino commented May 16, 2022 •

edited

Loading

iamNoah1 left a comment

iamNoah1 commented Jun 10, 2022

k8s-triage-robot commented Nov 25, 2022

rikatz commented Jan 8, 2023

rikatz commented Jan 8, 2023

linux-foundation-easycla bot commented Jan 14, 2023 •

edited

Loading

k8s-ci-robot commented Jan 14, 2023

chrisshino commented Jan 14, 2023

chrisshino commented Jan 14, 2023 •

edited

Loading

rikatz commented Jan 16, 2023

chrisshino commented Jan 21, 2023

k8s-ci-robot commented Jan 31, 2023

sherifkayad commented Aug 3, 2023 •

edited

Loading

Adds strict match annotation + updated tests/docs #8582

Are you sure you want to change the base?

Adds strict match annotation + updated tests/docs #8582

Conversation

chrisshino commented May 13, 2022 • edited Loading

What this PR does / why we need it:

Types of changes

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

k8s-ci-robot commented May 13, 2022

chrisshino commented May 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisshino May 16, 2022 • edited Loading

Choose a reason for hiding this comment

rikatz left a comment

Choose a reason for hiding this comment

chrisshino commented May 16, 2022

chrisshino commented May 16, 2022 • edited Loading

iamNoah1 left a comment

Choose a reason for hiding this comment

iamNoah1 commented Jun 10, 2022

k8s-triage-robot commented Nov 25, 2022

rikatz commented Jan 8, 2023

rikatz commented Jan 8, 2023

linux-foundation-easycla bot commented Jan 14, 2023 • edited Loading

k8s-ci-robot commented Jan 14, 2023

chrisshino commented Jan 14, 2023

chrisshino commented Jan 14, 2023 • edited Loading

rikatz commented Jan 16, 2023

chrisshino commented Jan 21, 2023

k8s-ci-robot commented Jan 31, 2023

sherifkayad commented Aug 3, 2023 • edited Loading

chrisshino commented May 13, 2022 •

edited

Loading

chrisshino May 16, 2022 •

edited

Loading

chrisshino commented May 16, 2022 •

edited

Loading

linux-foundation-easycla bot commented Jan 14, 2023 •

edited

Loading

chrisshino commented Jan 14, 2023 •

edited

Loading

sherifkayad commented Aug 3, 2023 •

edited

Loading