-
Notifications
You must be signed in to change notification settings - Fork 11
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.
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
Pass over caller identity information through the sansshell proxy #359
Merged
sfc-gh-srhodes
merged 13 commits into
Snowflake-Labs:main
from
stvnrhodes:proxy-identity
Nov 22, 2023
Merged
Pass over caller identity information through the sansshell proxy #359
sfc-gh-srhodes
merged 13 commits into
Snowflake-Labs:main
from
stvnrhodes:proxy-identity
Nov 22, 2023
Conversation
This file contains 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
For multi party authentication support, we plan on storing state in-memory in the sanshell server so the server needs to know the original caller's identity. Right now that gets lost when the caller goes through the proxy. This PR fixes that by including a json blob of marshalled rpcauth.PrincipalAuthInput information in the gRPC context sent from the proxy to the server. We use json and not base64-encoded proto because the size is unlikely to be significant and there's no existing common proto with the fields we want. I'm unconditionally sending the information when principal is populated so that we don't expose an excessive number of behavioral knobs. I've tweaked rpcauth.PeerInputFromContext so that it'll preserve information about the peer that gets added by rpcauth hooks. The API design intentionally tries to avoid exposing easy ways to use the gRPC metadata without checking its validity. We assume that in some cases sansshell server will have both proxied clients and direct clients. Most direct clients should be unable to pass in arbitrary proxied identity information. The interceptor for getting proxied identity is only implemented for unary RPCs because we don't yet have a use case for adding it to streaming RPCs. Part of Snowflake-Labs#346
stvnrhodes
force-pushed
the
proxy-identity
branch
from
October 20, 2023 03:01
b498a4f
to
10cd2fb
Compare
stvnrhodes
added a commit
to stvnrhodes/sansshell
that referenced
this pull request
Oct 24, 2023
These changes alongside Snowflake-Labs#351 are sufficient for MPA when using a direct connection to the server. Here's a few sample commands to try it out. ``` go run ./cmd/sansshell-server go run ./cmd/sanssh -mpa -targets localhost healthcheck validate go run ./cmd/sanssh -targets localhost mpa approve 12345 ``` This implements the client and server portion, but not the proxy portion. The proxy needs some additional features in core sansshell code before we can implement it. - Snowflake-Labs#361 for implementing the proxy equivalent of `ServerMPAAuthzHook()` - Snowflake-Labs#358 for implementing the proxy equivalents of `mpahooks.UnaryClientIntercepter()` and `mpahooks.StreamClientIntercepter()` - Snowflake-Labs#359 so that MPA can use the identity of the caller to the proxy instead of the identity of the proxy. Part of Snowflake-Labs#346
stvnrhodes
added a commit
to stvnrhodes/sansshell
that referenced
this pull request
Oct 24, 2023
These changes alongside Snowflake-Labs#351 are sufficient for MPA when using a direct connection to the server. Here's a few sample commands to try it out. ``` go run ./cmd/sansshell-server go run ./cmd/sanssh -mpa -targets localhost healthcheck validate go run ./cmd/sanssh -targets localhost mpa approve 12345 ``` This implements the client and server portion, but not the proxy portion. The proxy needs some additional features in core sansshell code before we can implement it. - Snowflake-Labs#361 for implementing the proxy equivalent of `ServerMPAAuthzHook()` - Snowflake-Labs#358 for implementing the proxy equivalents of `mpahooks.UnaryClientIntercepter()` and `mpahooks.StreamClientIntercepter()` - Snowflake-Labs#359 so that MPA can use the identity of the caller to the proxy instead of the identity of the proxy. Part of Snowflake-Labs#346
stvnrhodes
added a commit
to stvnrhodes/sansshell
that referenced
this pull request
Oct 26, 2023
These changes are sufficient for MPA when using a direct connection to the server. Here's a few sample commands you can run in parallel to try it out. ``` go run ./cmd/sansshell-server go run ./cmd/sanssh -client-cert ./auth/mtls/testdata/client.pem -client-key ./auth/mtls/testdata/client.key -mpa -targets localhost healthcheck validate go run ./cmd/sanssh -client-cert ./services/mpa/testdata/approver.pem -client-key ./services/mpa/testdata/approver.key -targets localhost mpa approve a59c2fef-748944da-336c9d35 ``` This implements the client and server portion, but not the proxy portion. The proxy part mostly builds on top of what I have here and will take advantage of some other features I'm implementing. - Snowflake-Labs#361 for implementing the proxy equivalent of `ServerMPAAuthzHook()` - Snowflake-Labs#358 for implementing the proxy equivalents of `mpahooks.UnaryClientIntercepter()` and `mpahooks.StreamClientIntercepter()` - Snowflake-Labs#359 so that MPA can use the identity of the caller to the proxy instead of the identity of the proxy. I'm going to wait to mention this in the readme until I've implemented the proxy part. Part of Snowflake-Labs#346
stvnrhodes
added a commit
to stvnrhodes/sansshell
that referenced
this pull request
Oct 26, 2023
These changes are sufficient for MPA when using a direct connection to the server. Here's a few sample commands you can run in parallel to try it out. ``` go run ./cmd/sansshell-server go run ./cmd/sanssh -client-cert ./auth/mtls/testdata/client.pem -client-key ./auth/mtls/testdata/client.key -mpa -targets localhost healthcheck validate go run ./cmd/sanssh -client-cert ./services/mpa/testdata/approver.pem -client-key ./services/mpa/testdata/approver.key -targets localhost mpa approve a59c2fef-748944da-336c9d35 ``` I've added some new testdata certs because I'm forbidding cases where approver == requester. I've updated the sansshell server code to allow any request if it's requested by our "normal" client cert and approved by our "approver" client cert. The output of `-mpa` prints a nonconfigurable help message to stderr while waiting on approval. If the command is already approved, the message won't show up. ``` $ sanssh -mpa -targets localhost healthcheck validate Multi party auth requested, ask an approver to run: sanssh --targets localhost:50042 mpa approve a59c2fef-748944da-336c9d35 Target localhost:50042 (0) healthy` ``` This implements the client and server portion, but not the proxy portion. The proxy part mostly builds on top of what I have here and will take advantage of some other features I'm implementing. - Snowflake-Labs#361 for implementing the proxy equivalent of `ServerMPAAuthzHook()` - Snowflake-Labs#358 for implementing the proxy equivalents of `mpahooks.UnaryClientIntercepter()` and `mpahooks.StreamClientIntercepter()` - Snowflake-Labs#359 so that MPA can use the identity of the caller to the proxy instead of the identity of the proxy. I'm going to wait to mention this in the readme until I've implemented the proxy part. Part of Snowflake-Labs#346
sfc-gh-srhodes
pushed a commit
that referenced
this pull request
Nov 10, 2023
These changes are sufficient for MPA when using a direct connection to the server. Here's a few sample commands you can run in parallel to try it out. ``` go run ./cmd/sansshell-server go run ./cmd/sanssh -client-cert ./auth/mtls/testdata/client.pem -client-key ./auth/mtls/testdata/client.key -mpa -targets localhost healthcheck validate go run ./cmd/sanssh -client-cert ./services/mpa/testdata/approver.pem -client-key ./services/mpa/testdata/approver.key -targets localhost mpa approve a59c2fef-748944da-336c9d35 ``` I've added some new testdata certs because I'm forbidding cases where approver == requester. I've updated the sansshell server code to allow any request if it's requested by our "normal" client cert and approved by our "approver" client cert. The output of `-mpa` prints a nonconfigurable help message to stderr while waiting on approval. If the command is already approved, the message won't show up. ``` $ sanssh -mpa -targets localhost healthcheck validate Multi party auth requested, ask an approver to run: sanssh --targets localhost:50042 mpa approve a59c2fef-748944da-336c9d35 Target localhost:50042 (0) healthy` ``` This implements the client and server portion, but not the proxy portion. The proxy part mostly builds on top of what I have here and will take advantage of some other features I'm implementing. - #361 for implementing the proxy equivalent of `ServerMPAAuthzHook()` - #358 for implementing the proxy equivalents of `mpahooks.UnaryClientIntercepter()` and `mpahooks.StreamClientIntercepter()` - #359 so that MPA can use the identity of the caller to the proxy instead of the identity of the proxy. Part of #346
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks mostly good. One question re: excessive context creation
sfc-gh-jallie
approved these changes
Nov 22, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
stvnrhodes
added a commit
to stvnrhodes/sansshell
that referenced
this pull request
Apr 5, 2024
I added these in Snowflake-Labs#359 in the original proxiedidentity implementation. In my initial draft I was using them as an additional authorization layer, but I switched to relying on the existing OPA policy authorization layer before submitting. Now, several months later, I've found another place where I want to use proxied identity data. The interceptors force folks to be intentional about allowing proxying, but in retrospect I think that adds friction without meaningfully helping security. I'd rather remove the need for them. I'm turning these interceptors into no-ops so that any existing references will continue to work. I've changed proxiedidentity.FromContext to directly work on grpc metadata.
stvnrhodes
added a commit
to stvnrhodes/sansshell
that referenced
this pull request
Apr 5, 2024
I added these in Snowflake-Labs#359 in the original proxiedidentity implementation. In my initial draft I was using them as an additional authorization layer, but I switched to relying on the existing OPA policy authorization layer before submitting. Now, several months later, I've found another place where I want to use proxied identity data. The interceptors force folks to be intentional about allowing proxying, but in retrospect I think that adds friction without meaningfully helping security. I'd rather remove the need for them. I'm turning these interceptors into no-ops so that any existing references will continue to work. I've changed proxiedidentity.FromContext to directly work on grpc metadata.
sfc-gh-srhodes
pushed a commit
that referenced
this pull request
Apr 8, 2024
I added these in #359 in the original proxiedidentity implementation. In my initial draft I was using them as an additional authorization layer, but I switched to relying on the existing OPA policy authorization layer before submitting. Now, several months later, I've found another place where I want to use proxied identity data. The interceptors force folks to be intentional about allowing proxying, but in retrospect I think that adds friction without meaningfully helping security. I'd rather remove the need for them. I'm turning these interceptors into no-ops so that any existing references will continue to work. I've changed proxiedidentity.FromContext to directly work on grpc metadata.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
For multi party authentication support, we plan on storing state in-memory in the sanshell server so the server needs to know the original caller's identity. Right now that gets lost when the caller goes through the proxy.
This PR fixes that by including a json blob of marshalled rpcauth.PrincipalAuthInput information in the gRPC context sent from the proxy to the server. We use json and not base64-encoded proto because the size is unlikely to be significant and there's no existing common proto with the fields we want. I'm unconditionally sending the information when principal is populated so that we don't expose an excessive number of behavioral knobs.
The API design intentionally tries to avoid exposing easy ways to use the gRPC metadata without checking its validity. We assume that in some cases sansshell server will have both proxied clients and direct clients. Most direct clients should be unable to pass in arbitrary proxied identity information.
I've updated the MPA service to make use of the proxied information. It's currently the only user.
Part of #346