Cosmos DB - Authorization policy #325

MindFlavor · 2021-07-05T16:39:27Z

This PR enhances the pipeline by adding the AuthorizationPolicy and migrates the preexisting CosmosDB pipeline operations to the new policy.

AuthorizationPolicy

The AuthorizationPolicy cannot be in Core since every service uses a different authorization mechanism. So this PR proposes to have a specific policy in each module (they can potentially be shared in case of duplication). This PR covers Cosmos DB only. While CosmosDB does not support (yet) the bearer token authentication, it's possibile to authenticate specific users with shared key.

The AuthorizationPolicy will handle the authentication by injecting the required headers to the request passing through the pipeline. Since the resulting authorization token can depend on the preexisting headers the AuthorizationPolicy must be executed right before the TransportPolicy. Also, since the token depends on the current timestamp (to mitigate reply attacks), AuthorizationPolicy must be run at every retry.

Cosmos context

In CosmosDB, the authorization token depends on the resource type. This information must be somehow passed through the pipeline because the AuthorizationPolicy needs it.
The information of which resource to authenticate must be passed by the operation builder prior starting the pipeline. For example, the create_database operation, knowing that it has to work on ResourceType::Databases, must be able to pass ResourceType::Databases to the AuthorizationPolicy. The pipeline has a Context struct that must be instantiated by the SDK user (ie the code that calls create_database). The Context will then traverse the pipeline. As such is a good candidate for the resource type information.

For this reason, this PR proposes an additional type, called PipelineContext that wraps the user-supplied Context and adds the module-dependent field. This is achieved by exposing a generic (constrained to be Send + Sync).

An alternative approach (shown here: https://github.com/MindFlavor/azure-sdk-for-rust/tree/cosmos/auth_policy%2Fdev) is to use Rust's runtime polymorphism in the form of std::any::Any. Basically Context will have a Box<dyn Any> field that can accept anything. It would be responsibility of the operation builder (ie the create_database function) to store the relevant information in the Box<dyn Any> instance. The AuthorizationPolicy will then downcast_ref() the Any instance and get the expected type.
While this approach is easy to do, it lacks any kind of type safety and relies upon conventions (which is very unidiomatic in Rust). Also, it's runtime enforced only (another thing unidiomatic in Rust).

The approach proposed in this PR, instead, is type safe and clearly self-explanatory.

Notable changes

Created the PipelineContext<BAG> discussed above (composition of Context and custom field of type BAG).
Changed Policy trait to be generic on the BAG above. This allows policies that accept generic PipelineContext<BAG>.
Changed the pipeline to be generic on the BAG above.
Fixed the preexisting policies to accept the BAG generic (called R for brevity, this is something we might want to correct to avoid confusion).
Implemented Policy<CosmosContext> for AuthorizationPolicy.
Changed pipeline code in CosmosDB to support the AuthorizationPolicy (and to send the proper PipelineContext<CosmosContext> down the pipeline.
Removed useless cosmos_client::prepare_request2.
Created a new function in cosmos_client called prepare_request_pipeline. This function will no longer perform authentication since it will delegated to the pipeline policy AuthorizationPolicy.
Moved all the authorization code from CosmosClient to AuthorizationPolicy (preexisting non-pipeline code has been rerouted pending removal).
Removed uninmplemented from add_as_header2 that caused E2E tests to fail (fix CosmosDB E2E attachment tests fail with not implemented panic #324).
Implemented add_as_header2 for all the required types.

rylev

I was skeptical of the proposal, but it's not too bad. I definitely think it's better than having an Any that service specific policies can downcast. I just had a bunch of small notes.

sdk/core/src/errors.rs

sdk/core/src/headers/mod.rs

sdk/cosmos/src/authorization_policy.rs

sdk/cosmos/src/clients/cosmos_client.rs

rylev · 2021-07-06T15:26:03Z

sdk/cosmos/src/clients/cosmos_client.rs

                resource_link,
                &time,
            )
        };
        self.prepare_request_with_signature(uri_path, http_method, &time, &auth)
    }

-    /// Prepares' an `azure_core::Request`.
+    /// Prepares' an `azure_core::Request`. This function will


nit: not sure if we need to make the comment lines so short. Means we quickly have lots of lines of comments.

I have a vim plugin that inserts a cr to keep comments < 80 chars wide (yes, I'm old 🤷‍♂️). I agree it doesn't really matter since Rust's lines can easily go over that limit. What should we use as a limit?

sdk/storage/src/data_lake/properties.rs

sdk/core/src/pipeline_context.rs

sdk/core/src/pipeline.rs

MindFlavor · 2021-07-06T16:14:34Z

Thank you @rylev for taking the time to review this! I'll try to address your suggestions and push another commit.

commit 79556d4 Author: Francesco Cogno <[email protected]> Date: Tue Jul 13 10:09:16 2021 +0200 docs on pipeline commit 4a95aad Author: Francesco Cogno <[email protected]> Date: Tue Jul 13 09:59:02 2021 +0200 R -> C generic commit cac15f7 Author: Francesco Cogno <[email protected]> Date: Tue Jul 13 09:52:55 2021 +0200 better comments commit f047a82 Author: Francesco Cogno <[email protected]> Date: Mon Jul 12 21:11:21 2021 +0200 docs commit 876191c Author: Francesco Cogno <[email protected]> Date: Mon Jul 12 18:52:27 2021 +0200 bag to contents commit 62b7c76 Author: Francesco Cogno <[email protected]> Date: Mon Jul 12 18:43:46 2021 +0200 data_lake commit eff9c2a Author: Francesco Cogno <[email protected]> Date: Mon Jul 12 18:23:04 2021 +0200 from str to TimeNonce commit 7b63386 Author: Francesco Cogno <[email protected]> Date: Wed Jul 7 19:09:24 2021 +0200 rewritten code commit 2ad67a2 Author: Francesco Cogno <[email protected]> Date: Tue Jul 6 18:46:21 2021 +0200 removed unwrap commit 7105dab Author: Francesco Cogno <[email protected]> Date: Tue Jul 6 18:14:57 2021 +0200 errors to lowercase

rylev

@MindFlavor looks good. Shall we merge?

MindFlavor · 2021-07-21T16:16:17Z

@MindFlavor looks good. Shall we merge?

Yes, let's keep the ball rolling! 👍

Second draft

51b87de

MindFlavor added feature-request This issue requires a new behavior in the product in order be resolved. Client This issue points to a problem in the data-plane of the library. Cosmos The azure_cosmos crate pipeline labels Jul 5, 2021

rylev suggested changes Jul 6, 2021

View reviewed changes

rylev approved these changes Jul 21, 2021

View reviewed changes

MindFlavor merged commit 08090ef into Azure:master Jul 21, 2021

MindFlavor deleted the cosmos/auth_policy/generic/pr branch July 21, 2021 16:16

heaths added Azure.Core The azure_core crate design-discussion An area of design currently under discussion and open to team and community feedback. labels Jan 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cosmos DB - Authorization policy #325

Cosmos DB - Authorization policy #325

MindFlavor commented Jul 5, 2021

rylev left a comment

rylev Jul 6, 2021

MindFlavor Jul 12, 2021

MindFlavor commented Jul 6, 2021

rylev left a comment

MindFlavor commented Jul 21, 2021

Cosmos DB - Authorization policy #325

Cosmos DB - Authorization policy #325

Conversation

MindFlavor commented Jul 5, 2021

AuthorizationPolicy

Cosmos context

Notable changes

rylev left a comment

Choose a reason for hiding this comment

rylev Jul 6, 2021

Choose a reason for hiding this comment

MindFlavor Jul 12, 2021

Choose a reason for hiding this comment

MindFlavor commented Jul 6, 2021

rylev left a comment

Choose a reason for hiding this comment

MindFlavor commented Jul 21, 2021