Repository navigation
Warn when a policy row matches on an unverified email - #594
Conversation
The built-in email matcher does not look at email_verified, unlike the plugin examples. An OP that is authoritative for a domain may omit the claim or set it false and still be the right source of truth, so this allows the match as before and logs a warning instead of denying, per the shape suggested in the issue. Adds an EmailVerified pointer to the checked claims so an absent claim is distinguishable from a false one, and a docs note pointing admins at sub matching or a policy plugin if they want this enforced. Fixes #591 Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
0a7c3ba to
62eb7bb
Compare
Signed-off-by: Ethan Heilman <ethan.r.heilman@gmail.com>
62eb7bb to
d6d4f82
Compare
| Email string `json:"email"` | ||
| // EmailVerified is a pointer so we can tell "claim absent" from | ||
| // "claim present and false". Not all OPs send it. | ||
| EmailVerified *bool `json:"email_verified"` |
There was a problem hiding this comment.
Unfortuantely some OPs like to use a string here so
"email_verified" : "true" ("true" is a string)
rather than
"email_verified" : true (true is a bool)
So to prevent JSON unmarshalling errors we can not assume that email_verified is of type bool despite the fact that email_verified should clearly be of type bool. =(
There was a problem hiding this comment.
Good catch, thanks. Dropped the typed field entirely and read the claim out of ExtraClaims instead, since UnmarshalJSON already normalizes both shapes to a string (fmt.Sprint on the bool, the string as-is). Comparison is strings.EqualFold(v, "true"), so true, "true" and "True" all count as verified. Test table now covers claim absent, bool true/false and string true/false/True.
While testing the string case I hit something related: policy/plugins/tokens.go:49 declares EmailVerified *bool, so an OP that sends "email_verified": "true" fails the whole payload there:
Error checking policy plugins: error unmarshalling pk token payload: json: cannot unmarshal string into Go struct field Claims.email_verified of type bool
That kills every policy plugin for that OP, not just the email_verified value. It's on main today, independent of this PR. Want me to send a separate PR for it? Happy to, just did not want to widen this one without asking.
There was a problem hiding this comment.
That's a bug worth fixing.
I was also wondering about userinfo endpoint as well.
Want me to send a separate PR for it? Happy to, just did not want to widen this one without asking.
You can do it in this PR or in a separate one. Up to you.
There was a problem hiding this comment.
Opened #595 for it, kept it out of this PR so this one stays about the warning.
On userinfo: it looks fine. Plugins get the userinfo document as an opaque string in OPKSSH_PLUGIN_USERINFO, nothing parses it, and the enforcer unmarshals it into checkedClaims, which has no typed email_verified field, so a string value there does not fail either. #595 has a case that sends the string form in both the ID Token and the userinfo JSON to keep it that way.
One judgement call in #595 worth your eye: I left a numeric email_verified still erroring, since there is an existing test asserting that for 1234. Easy to change if you would rather nothing about that claim can fail the payload.
There was a problem hiding this comment.
Going to merge this PR unless you want to make any other changes
Agreed. It is unfortunate that OPs aren't better about when they are and are not authoritative for an email and instead use For instance GCP services are assigned an email as their identity, but the email they are assigned does not have a real email address (I tried emailing it once), so they can't be verified via email. Thankfully GCP does the right thing and sets {
"aud": "example-audience",
"azp": "112010400000000710080",
"email": "service-account@example.iam.gserviceaccount.com",
"email_verified": true,
"exp": 1745365618,
"iat": 1745362018,
"iss": "https://accounts.google.com",
"sub": "112010400000000710080"
} |
Some OPs send email_verified as the string "true" rather than a bool, so unmarshalling it into a *bool would fail the whole payload. The normalized ExtraClaims map already turns both shapes into a string, so read it from there and compare case-insensitively. Tests cover bool and string forms of the claim. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
##### [\`v0.17.0\`](https://github.com/openpubkey/opkssh/releases/tag/v0.17.0) ##### 🚀 Features - feat: support CI/CD login from Forgejo Actions (Codeberg) [@Blacks-Army](https://github.com/Blacks-Army) ([#580](openpubkey/opkssh#580)) - ssh-agent off by default [@EthanHeilman](https://github.com/EthanHeilman) ([#626](openpubkey/opkssh#626)) - feat: add GitLab CI provider support [@cgroschupp](https://github.com/cgroschupp) ([#559](openpubkey/opkssh#559)) - fix(verify): Rejects unsafe principals in authorized_keys [@EthanHeilman](https://github.com/EthanHeilman) ([#597](openpubkey/opkssh#597)) - Warn when a policy row matches on an unverified email [@arpitjain099](https://github.com/arpitjain099) ([#594](openpubkey/opkssh#594)) - Support sshd_session_t SELinux domain in installer [@ozan956](https://github.com/ozan956) ([#614](openpubkey/opkssh#614)) - login: load certificate into ssh-agent with configurable lifetime [@sini](https://github.com/sini) ([#611](openpubkey/opkssh#611)) - policy/plugins: marshal GROUPS and AUD env JSON with encoding/json [@SashaMIT](https://github.com/SashaMIT) ([#601](openpubkey/opkssh#601)) ##### 🐛 Bug Fixes - fix(verify): Rejects unsafe principals in authorized_keys [@EthanHeilman](https://github.com/EthanHeilman) ([#597](openpubkey/opkssh#597)) - fix: Policy footguns [@EthanHeilman](https://github.com/EthanHeilman) ([#596](openpubkey/opkssh#596)) - fix: Regression test for --private-key-file arg higher precedence over \~/.ssh/opkssh [@EthanHeilman](https://github.com/EthanHeilman) ([#592](openpubkey/opkssh#592)) - Accept a string email_verified claim in the plugin env vars [@arpitjain099](https://github.com/arpitjain099) ([#595](openpubkey/opkssh#595)) - Deny malformed oidc policy rows instead of panicking [@arpitjain099](https://github.com/arpitjain099) ([#593](openpubkey/opkssh#593)) ##### Chores - dep: Update to OpenPubkey v0.27.0 [@EthanHeilman](https://github.com/EthanHeilman) ([#598](openpubkey/opkssh#598)) - fix(deps): Update release-drafter/release-drafter action to v7.7.0 @[renovate\[bot\]](https://github.com/apps/renovate) ([#590](openpubkey/opkssh#590)) - fix(deps): Update actions/setup-go action to v7 @[renovate\[bot\]](https://github.com/apps/renovate) ([#578](openpubkey/opkssh#578)) - fix(deps): Update release-drafter/release-drafter action to v7.6.0 @[renovate\[bot\]](https://github.com/apps/renovate) ([#581](openpubkey/opkssh#581)) - \[fix] Windows linter errors [@EthanHeilman](https://github.com/EthanHeilman) ([#577](openpubkey/opkssh#577)) - fix(deps): Update zizmorcore/zizmor-action action to v0.6.0 @[renovate\[bot\]](https://github.com/apps/renovate) ([#575](openpubkey/opkssh#575)) - fix(deps): Update Docker @[renovate\[bot\]](https://github.com/apps/renovate) ([#568](openpubkey/opkssh#568)) - Bump testcontainers-go to 0.43.0 [@EthanHeilman](https://github.com/EthanHeilman) ([#571](openpubkey/opkssh#571)) - Update CLI documentation @[github-actions\[bot\]](https://github.com/apps/github-actions) ([#627](openpubkey/opkssh#627)) - fix(deps): Update dependency ubuntu to v26 @[renovate\[bot\]](https://github.com/apps/renovate) ([#616](openpubkey/opkssh#616)) - chore(deps): bump github.com/moby/go-archive from 0.2.0 to 0.3.0 @[dependabot\[bot\]](https://github.com/apps/dependabot) ([#608](openpubkey/opkssh#608)) - fix: reject empty identity attributes in policy [@SashaMIT](https://github.com/SashaMIT) ([#603](openpubkey/opkssh#603)) - fix(deps): Update go toolchain directive to v1.26.6 @[renovate\[bot\]](https://github.com/apps/renovate) ([#605](openpubkey/opkssh#605)) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Fixes #591, taking the second one off your plate since you are busy this week.
This follows the shape you laid out in the issue rather than my original suggestion. Nothing is denied that was allowed before. When a policy row matches on the email claim (exact or
oidc-match-end:email:) and the ID Token either has noemail_verifiedclaim or sets it to false, opkssh allows the login and logs a warning.email_verified: trueandsubmatches log nothing.checkedClaims.EmailVerifiedis a*boolso an absent claim is distinguishable from a present false one, and the two cases get different warning text.I left out any config knob. Your note said admins who want this enforced should use a plugin, and a knob is easy to layer on later if you decide you want one.
Also added a short paragraph to docs/config.md next to the email matching section: matching on email trusts the OP to be authoritative for it, opkssh warns rather than enforces, and if you do not trust an OP for email use
subor a plugin.Tests:
TestEmailVerifiedWarningcovers claim absent, false and true, asserting both that access is still granted and what does or does not get logged.TestEmailVerifiedNoWarningOnSubMatchpins that a sub match stays quiet. The absent and false cases fail on main.go test ./...passes.