Add Snowflake credential issuance - #144
Open
santhosh-c1 wants to merge 14 commits into
Open
Conversation
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Connector PR Review: Add Snowflake credential issuanceBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe new commits ( Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Contributor
|
Squire (openai/gpt-5.6-terra): The credential-issuance implementation and its lifecycle coverage are on the right track. One service-user detail needs a follow-up: Snowflake requires I opened #145 against this PR branch. It uses the user’s default role after verifying the grant, fails before token creation when no suitable role exists, and adds the |
Programmatic access tokens could never be discovered by a sync. The syncer walks a child resource type once per parent only when the parent carries a ChildResourceType annotation, and the user resource annotated only rsa_public_key. The sole remaining call is the top-level one with a nil parent, which the token builder returns nothing for, so no token ever appeared in inventory no matter how many existed -- while the issuer advertised CREDENTIAL_RESOURCE_MODE_DISCOVERABLE. That also meant an issued token could not be found later in order to revoke it. Verified against a live Snowflake account: before, a sync over three users walked programmatic_access_token exactly once and reported zero with a token demonstrably present; after, it walks once per user and finds it. The full issue -> sync -> DeleteResourceV2 -> sync lifecycle now passes. Also in this change: - Remove the token when any step after creation fails. The plaintext is discarded and no secret resource is recorded on those paths, and the SDK does not retry issuance, so the credential was left live with nothing holding a handle to revoke it. Cleanup is detached from the request context so it still runs when the caller's context is done. - Fix the unreachable "no default role" check. The SQL API returns every column as text, so an unset DEFAULT_ROLE arrives as the literal string "null" rather than empty. The absence test never matched, and callers were sent to the other branch and told to grant a role named "null". Normalizing goes through the existing rowNull constant rather than a second spelling. - Read the issued token secret by column name instead of by position, so a reordered or inserted column fails loudly rather than silently returning another field as the credential. Confirmed against a live response that the column is token_secret. - Skip a user whose tokens the connector's role may not read (422/003001) instead of failing the whole sync, matching the existing handling in pkg/connector/rsa.go. - Regenerate baton_capabilities.json and document token issuance, including Snowflake's requirement that the user have a network policy attached before it will mint a token. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CreateProgrammaticAccessToken formatted the whole statement, then discarded it and formatted it again when a role restriction was present. Both the format string and its arguments were duplicated, so any future change to the statement had to be made in two places or silently apply to only one of them. Build the optional clause instead and interpolate it, which keeps a single source of truth for the statement shape. The clause cannot simply be appended to the finished statement: that string ends with a semicolon, so the appended text lands after the terminator and Snowflake rejects it with a SQL compilation error. Interpolating keeps ROLE_RESTRICTION in its documented position ahead of DAYS_TO_EXPIRY. Snowflake does accept the two clauses in either order, but relying on that would be relying on undocumented behaviour for no benefit. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The role-restricted statement was covered only indirectly, by asserting that a substring appeared somewhere in the statements a mock server received, and the unrestricted statement was not covered at all. The statement construction was just reworked, so pin both shapes to their exact SQL. These also guard the specific mistake the rework avoids: appending the role clause to the finished statement puts it after the terminating semicolon, which Snowflake rejects with a SQL compilation error. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…-restriction Fix service-user PAT role restriction
main added CAPABILITY_ACTIONS to baton_capabilities.json in #146 while this branch added CAPABILITY_CREDENTIAL_ISSUE to the same list, so the generated file conflicted. Resolved by regenerating it from the merged tree rather than by hand, so the committed metadata is what the binary actually produces. It now carries both capabilities. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
executeStatement classified a Snowflake access-control denial only on the POST leg. A statement that goes async reports its outcome on the follow-up GET instead, and that leg returned the raw error, so a 422/003001 arriving there was indistinguishable from a real failure and aborted the sync rather than skipping the object the connector's role cannot see. Both legs now go through classifyStatementError. The regression test drives a denial down each leg independently and fails on the GET case without this change. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two defects in the service-user issuance path, both confirmed against a live Snowflake account. Quoted identifiers. SHOW GRANTS TO USER wraps a mixed-case or spaced identifier in double quotes, while DESCRIBE USER reports DEFAULT_ROLE bare. Comparing the two raw strings reported a granted role as ungranted, so issuance failed with "default role %q is not granted to the user" for a role that was granted, and no user whose default role is mixed-case or spaced could be issued a token at all. Both sides now go through unquoteSnowflakeIdentifier and compare case-insensitively. Role on user mutations. ALTER USER ... ADD/REMOVE PROGRAMMATIC ACCESS TOKEN ran with no role, so it executed under the session's default role. SetUserDisabled, CreateUserREST and DeleteUserREST all force USERADMIN precisely because the session default is not guaranteed to hold ALTER USER on other users; these two statements now do the same. Reads are untouched and still run as the session role. Verified live: with the target's default role set to "Mixed Case Role", issuance failed before this change and the full issue -> delete -> verify-absent lifecycle passes after it. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ement-error-classification Fix service-user issuance: quoted roles, USERADMIN on mutations, async denial classification
Issue() sampled time.Now() before DESCRIBE USER, SHOW GRANTS TO USER and the ALTER USER round-trip, but Snowflake derives a token's expiry from its own clock at ALTER USER time. The real expiry is therefore later than the computed one by however long the pre-flight took, so a request whose remaining time sits just above a whole number of days trips the "provider expiry exceeds requested" guard and the cleanup defer destroys a token that was fine. Sampling after the pre-flight leaves only the create round-trip inside the window and yields a shorter token instead of a failed issuance. The post-creation read-back and the default-role pre-check both ran as the session's default role while creation forces USERADMIN. SHOW USER PROGRAMMATIC ACCESS TOKENS FOR USER needs ownership or MONITOR on the target user and SHOW GRANTS TO USER needs its own privileges, neither of which creating a token requires. A role holding one and not the other created a good credential and immediately destroyed it, so issuance could never succeed for that tenant. Both now degrade on 422/003001: the role check is skipped and Snowflake is left to reject the statement itself, and the read-back falls back to the locally computed expiry, which is never later than the provider's. Replaces the package-level lastRole in the Snowflake tests with a mutex-guarded per-test recorder, which removes the order dependency between subtests and lets them run in parallel. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e-expiry-and-readback fix(pat): sample the expiry clock late and survive read-back denials
…note Both privilege degradations added in #149 logged at Debug, so at the default level an operator saw nothing distinguishing a locally computed expiry from one Snowflake actually returned. Per the repo's log-level rules a skip-and-continue degradation is Warn, and both fire once per issuance rather than per resource, so there is no volume concern. The read-back message now also carries the estimated expiry it substituted. The token builder's own skip in ListProgrammaticAccessTokens stays at Debug: that one fires once per user per sync and is the per-resource case the same rules keep quiet. The service-user note in the docs still claimed the connector verifies the default-role grant before issuing. That has been best-effort since #149, so it is softened to match. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…adation-visibility fix(pat): surface issuance degradations at Warn and correct the docs note
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Live tenant verification is pending Snowflake credentials.