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

ROX-27432: allow regex in authentication claimrules #1441

Merged
merged 4 commits into from
Dec 18, 2024
Merged

ROX-27432: allow regex in authentication claimrules #1441

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b909935
allow regex in claimrules
tommartensen Dec 17, 2024
b311417
hide port-forward logs
tommartensen Dec 17, 2024
0fa5429
fix
tommartensen Dec 17, 2024
2eaa54d
add more tests to cover substring matches
tommartensen Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions .github/workflows/PR.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ jobs:
run: |
ENVIRONMENT=development TEST_MODE=true make install-argo clean-argo-config install-monitoring helm-deploy
sleep 10 # wait for old pods to disappear so the svc port-forward doesn't connect to them
kubectl -n infra port-forward svc/infra-server-service 8443:8443 &
kubectl -n infra port-forward svc/infra-server-service 8443:8443 > /dev/null 2>&1 &
sleep 10

kubectl -n infra logs -l app=infra-server --tail=-1
Expand All @@ -115,7 +115,7 @@ jobs:

- name: Check the deployment
run: |
kubectl -n infra port-forward svc/infra-server-service 8443:8443 &
kubectl -n infra port-forward svc/infra-server-service 8443:8443 > /dev/null 2>&1 &
sleep 10

version="$($INFRACTL version --json)"
Expand Down Expand Up @@ -157,7 +157,7 @@ jobs:
env:
INFRA_TOKEN: ${{ secrets.INFRA_TOKEN_DEV }}
run: |
kubectl -n infra port-forward svc/infra-server-service 8443:8443 &
kubectl -n infra port-forward svc/infra-server-service 8443:8443 > /dev/null 2>&1 &
sleep 5

$INFRACTL whoami || true
Expand All @@ -173,6 +173,6 @@ jobs:
env:
INFRA_TOKEN: ${{ secrets.INFRA_TOKEN_DEV }}
run: |
kubectl -n infra port-forward svc/infra-server-service 8443:8443 &
kubectl -n infra port-forward svc/infra-server-service 8443:8443 > /dev/null 2>&1 &
sleep 5
make go-e2e-tests
8 changes: 7 additions & 1 deletion auth/claimrule/claim_rule.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"encoding/base64"
"encoding/json"
"fmt"
"regexp"
"strings"

"github.com/jeremywohl/flatten/v2"
Expand Down Expand Up @@ -70,9 +71,14 @@ func (cr *ClaimRule) equalCheck(flatTokenClaims map[string]interface{}, jsonPath
return errors.Errorf("expected claim %q is not found", jsonPath)
}

if cr.Value != tokenClaimValue {
pattern := fmt.Sprintf("^%s$", cr.Value)
found, err := regexp.MatchString(pattern, tokenClaimValue.(string))
if !found {
return errors.Errorf("expected claim %q is not correct", jsonPath)
}
if err != nil {
return errors.Wrapf(err, "error matching claim %s to expected value", tokenClaimValue)
}

return nil
}
Expand Down
33 changes: 33 additions & 0 deletions auth/claimrule/claim_rule_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,39 @@ func getDataSets() map[string]dataSet {
}},
err: true,
},
"eq-regex-match": {
tokenClaims: map[string]interface{}{
"field": "val1",
},
rules: ClaimRules{{
Value: "(val1|val2)",
Path: "field",
Op: "eq",
}},
err: false,
},
"eq-regex-no-match": {
tokenClaims: map[string]interface{}{
"field": "val3",
},
rules: ClaimRules{{
Value: "(val1|val2)",
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be useful to test that val3xyz does not trigger as a match, and prevent a regression that would allow subset matches?

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 added additional substring tests in 2eaa54d.

I am preventing substring matching by wrapping the test string with ^$: https://github.com/stackrox/infra/pull/1441/files#diff-4686f933d92fd202f84313c97ceb4ee8b35f5eb949a71b7ab8a6687a452947daR74

Copy link
Contributor

Choose a reason for hiding this comment

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

🥳 thx. I saw you had the ^string$ but I like that the new tests can catch if that regresses/changes someday.

Path: "field",
Op: "eq",
}},
err: true,
},
"in-regex-match": {
tokenClaims: map[string]interface{}{
"field": []string{"val1", "val2"},
},
rules: ClaimRules{{
Value: "(val2|val3)",
Path: "field",
Op: "in",
}},
err: false,
},
}
}

Expand Down
Loading