Skip to content

Conversation

sfc-gh-mwalas
Copy link
Collaborator

Currently httpoverrpc is unusable with MPA as every single request need to be separately approved. This change ignores requests to the server while constructing MPA.

@@ -106,7 +106,6 @@ allow {

# Allow MPA setting when not sending a proxied identity. The proxy is allowed above.
allow {
not input.metadata["proxied-sansshell-identity"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you also update

If you use OPA for authz, you'll also need to update the server's rego policies to reject requests that unexpectedly set `proxied-sansshell-identity` metadata if it allows callers other than the proxy to make direct calls. If you fail to do so, a direct caller can manipulate the metadata to approve their own request. For example, the policy below will reject calls if proxied identity information is in the metadata and the caller is something other than a peer with an identity of `"proxy"`.
and https://github.com/Snowflake-Labs/sansshell/pull/562/files#diff-2604a9e725510ddcf1204e208d3d7ec2894cdcd4274c856c1a45df50128c4085L107 please?

Another q: after this change, the MPA approval will be valid for any requests to the same {hostname, port, tlsconfig, and protocol} correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, I got this just for testing purposes in place, we shouldn't merge this change to default policy.

Re the second point, indeed the MPA for httpoverrpc will become valid after this change for all combination of {hostname, port, tlsconfig, and protocol}, after a debate on what are meaningful levels of authz policy for MPA here we ended up saying that for httpoverrpc current MPA of each request does not make much sense.

@@ -0,0 +1,27 @@
/* Copyright (c) 2019 Snowflake Inc. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

update copyright year

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants