Repository navigation
CXP-1250 fix: report real last-used dates for RSA keys and programmatic access tokens - #165
JavierCarnelli-ConductorOne wants to merge 6 commits into
Conversation
RSA keys reported the time the key was set as their last-used date, which made actively used keys look unused. The set time is now the key's creation date, and last-used dates for RSA keys and programmatic access tokens come from SNOWFLAKE.ACCOUNT_USAGE.CREDENTIALS, read once per sync and cached in the session store. Without the SECURITY_VIEWER database role the sync still succeeds, emits keys and tokens without a last-used date, and logs one warning per sync. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rows without LAST_USED_ON carry no date to report, so filtering them out reduces the rows, partitions and cache writes per sync. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Superseded — see the current review report for commit
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
No blocking issues found — see the full review report
The credentials query is the first required sync statement that needs a warehouse. Treat Snowflake's 000606 (no usable warehouse) like the privilege denial: warn once per sync and emit RSA keys and tokens without a last-used date instead of failing List. Document the USAGE grant on the default warehouse. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
JavierCarnelli-ConductorOne
changed the title
Superseded — see the current review report for commit
|
|
|
||
| if c.needsStatementResultFetch(ctx, resp, &response, "ListCredentials") { | ||
| req, err = c.GetStatementResponse(ctx, response.StatementHandle) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| resp2, err := c.Do(req, uhttp.WithJSONResponse(&response), uhttp.WithErrorResponse(&apiErr)) | ||
| defer closeResponseBody(resp2) | ||
| if err != nil { | ||
| return classifyCredentialsError(resp2, &apiErr, err) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): this is the first query here that runs on a warehouse against ACCOUNT_USAGE, so it can easily outlast the synchronous window, especially when the warehouse has to resume first. If the single follow-up GET also returns 202 (still running), response has no row-type metadata, no data and no partitions. listCredentials then returns nil and loadCredentials caches loaded with no rows. Every RSA key and PAT then syncs without a last-used date for the whole sync, and no warning is logged. Consider polling the handle until it returns 200, or checking resultSetInline() after the GET and returning an error or caching unavailable with a warning.
There was a problem hiding this comment.
Accepted risk, documented on listCredentials in adc406a. The view is small, so this needs an unusually slow warehouse, and the last-used dates are optional.
There was a problem hiding this comment.
Could we add a debug log when resultSetInline() is false after the follow-up GET? I think this may happen more often than we're assuming.
Per the SQL API docs, the POST returns 202 once the statement runs past 45 seconds. A GET on a statement that's still running also returns 202 right away instead of waiting. We don't poll or set a timeout, so any run over ~45s almost certainly loses the dates. That includes time spent waiting for a shared default warehouse or for a suspended one to resume, not only a slow query. When it happens, the sync caches loaded with no rows and logs nothing, which looks the same as "nothing was ever used".
A log line with the statement handle would at least show us how often this happens in real tenants, so we can decide later whether polling is worth it.
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
No blocking issues found — see the full review report
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
No blocking issues found — see the full review report
| GRANT DATABASE ROLE SNOWFLAKE.SECURITY_VIEWER TO ROLE <connector-role>; | ||
| ``` | ||
|
|
||
| This role is read-only, but it also lets the connector's role read other security information in Snowflake, such as login history. Without it, syncs still work, but these credentials have no last-used date, so C1 never flags them as unused. Reading these dates runs a query on the service account's default warehouse, so the connector's role also needs `USAGE` on that warehouse, with the same fallback if it doesn't. Snowflake can take up to 2 hours to record a credential's latest use. For RSA keys, the date the key was set in Snowflake appears as its creation date. |
There was a problem hiding this comment.
[Docs] The "C1 never flags them as unused" claim isn't backed by the connector
The connector only leaves LastUsedAt unset. What C1 does with a secret that has no last-used date is decided on the platform side. Describe what the connector does instead:
Without it, syncs still work, but these credentials are synced without a last-used date.RSA List now returns early for users with no key in either slot, so they no longer cost session store reads, and a tenant without RSA keys or token sync no longer runs the warehouse query. Without a session store, UserCredentials now skips the optional dates and logs one warning instead of scanning the whole view on every call. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- UserCredentials drops its unused bool result, and its doc now names the statement and the privileges it needs. - Errors from the credentials client no longer repeat the connector prefix that the builders already add. - fetchRemainingPartitions reuses fetchPartitionRows instead of duplicating it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Superseded — see the current review report for commit
|
Connector PR Review: CXP-1250 fix: report real last-used dates for RSA keys and programmatic access tokensBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThe new commit (
I scanned the full PR diff for security and correctness. Repo-local criteria were applied:
The tests now pass a real in-memory session store and cover the nil-session skip and the user-without-keys skip. The incremental diff was complete (not partial). Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agentsReviewed commit: |
There was a problem hiding this comment.
No blocking issues found — see the full review report
|
|
||
| l.Debug("UserRsa", zap.Any("user", user)) | ||
|
|
||
| if user.RsaPublicKeyFp == "" && user.RsaPublicKeyFp2 == "" && |
There was a problem hiding this comment.
Non-blocking, fine as a follow-up. This check could move up a level so the RSA and PAT child List calls never run for users without credentials.
RSA: SHOW USERS already returns has_rsa_public_key, and we parse it into User.HasRSAPublicKey. userResource adds the RSA ChildResourceType annotation for every user when secrets sync is on (users.go:247), so every user costs one DESCRIBE USER. Gating it would skip that for keyless users:
if secrets.syncSecrets && user.HasRSAPublicKey {
opts = append(opts, rs.WithAnnotation(&v2.ChildResourceType{ResourceTypeId: rsaPublicKeyResourceType.Id}))
}Users with a key still need DESCRIBE USER, since the fingerprints and set times only come from there. This early return would stay as a safety net.
PATs: SHOW USERS also has a has_pat column ("If TRUE, the user has one or more programmatic access tokens", docs). We don't parse it today. Gating the PAT annotation on it would skip SHOW USER PROGRAMMATIC ACCESS TOKENS for users without tokens. Two things to handle:
- Read it as an optional column, like
created_oninListProgrammaticAccessTokens, and default totruewhen it's missing. Adding it touserStructFieldToColumnMapwould makeParseRowfail on accounts that don't return it. DESCRIBE USERdoesn't return it, soGetUser(used byCreateAccount) should also default totrue.
Worth checking on a test account first:
- Is
has_rsa_public_keytrue when onlyRSA_PUBLIC_KEY_2is set? - Does
has_patcount disabled or expired tokens?
If either is false, gating would drop those credentials from the sync.
agustin-conductor
left a comment
agustin-conductor
left a comment
There was a problem hiding this comment.
Comment about optionally skipping child resource type for users is a non blocking suggestion, it is technically pre-existing this PR.
Closes CXP-1250.
Summary
RSA public key secrets reported
RSA_PUBLIC_KEY_LAST_SET_TIMEas their last-used date, so C1 showed when a key was set, not when it was used. Programmatic access tokens (PATs) had no last-used date at all. Either way, C1 could not tell which credentials were actually unused.This PR reads the real last-use from
SNOWFLAKE.ACCOUNT_USAGE.CREDENTIALSand attaches it to both secret types.Changes
Reading the credentials view (
pkg/snowflake/credentials.go)Client.UserCredentialsruns one query per sync forKEYPAIRandPATrows that have aLAST_USED_ON.RSA keys (
pkg/connector/rsa.go)DESCRIBE USERnow also readsRSA_PUBLIC_KEY_FP/RSA_PUBLIC_KEY_2_FP. Each key is matched to its credentials row by normalized fingerprint (theSHA256:prefix is dropped).LastUsedAtcomes from the matched row'sLAST_USED_ON. The last-set time is now reported as the resource's created-at.Programmatic access tokens (
pkg/connector/programmatic_access_tokens.go)LastUsedAtis set fromLAST_USED_ON.created_onfromSHOW USER PROGRAMMATIC ACCESS TOKENSis now reported as the resource's created-at.Issuecarry neither date.When the dates can't be read, the sync still succeeds
003001, or002003when it has no access to theSNOWFLAKEdatabase), or there is no usable warehouse to run the query (000606), the result is cached as unavailable for the sync. One warning is logged with the matching fix, and RSA keys and PATs are synced without a last-used date.List, as before. No resources are dropped.Docs (
README.md,docs/connector.mdx)SECURITY_VIEWERgrant needed for last-used dates.GRANT USAGE ON WAREHOUSE.Behavior changes
SHOW/DESCRIBEcalls don't. Each sync may resume the default warehouse once, which Snowflake bills at a minimum of 60 seconds.LastUsedAtchanges meaning: it now means actual use. Keys that have never been used, or whose use isn't visible, have no last-used date instead of their set time.Notes for reviewers
DELETED_ONfilter on purpose.ACCOUNT_USAGE.CREDENTIALShas no such column, and Snowflake documents that the view "does not include credentials that have been deleted" (docs). Adding the filter would make the query fail.Testing
003001/002003/000606(cached, one warning per sync), other errors failing and not being cached, and fingerprint and name matching for RSA keys and PATs.go test ./...andgolangci-lint(v2.15.0) pass.🤖 Generated with Claude Code