Skip to content
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

Generic Custom Pre-Processing Traits & Caller Identity Modeling? #2177

Open
commiterate opened this issue Mar 11, 2024 · 2 comments
Open

Generic Custom Pre-Processing Traits & Caller Identity Modeling? #2177

commiterate opened this issue Mar 11, 2024 · 2 comments

Comments

@commiterate
Copy link

commiterate commented Mar 11, 2024

Overview

Would it make sense to add a generic custom pre-processing trait like @refine(functionId: String | Enum, protocols: List[shapeId]) to the core library? This can signal server stub code generators to make users register pre-processing functions which extend their built-in deserialization systems.

From a UX perspective, this lets users skip writing their own Smithy traits and the supporting code generator logic for custom type refinement and constraint traits. For users not comfortable with writing or extending code generators, this eliminates a lot of friction (kind of an extension of #117).

Background

Today, Smithy provides various type refinement and constraint traits in its core library. These traits are effectively a set of standard pre-processing directives. For example:

  • @default
    • Injects static default values (null safety in a sense).
  • @required
    • Null check.
  • @jsonName
    • Maps JSON payload fields to the modeled input (override guessed JSON field name).
  • @httpHeader
    • Maps HTTP header fields to the modeled input.

To provide a batteries-included experience, server stub code generators can augment built-in deserialization mechanisms to automatically handle these. Support across all code generators will generally be strongest for core library traits and weaker for custom traits (e.g. framework-specific).

I'm including validation in deserialization because stuff like field length validation is really just type information that's expressed as validation logic. This is done because the underlying programming language's type system isn't expressive enough to define, for example, a string type with length bounds (e.g. String<Length<2,7>>) or it's cumbersome to work with.

Unmodeled information, however, has a rougher experience.

For unmodeled traits or ones unsupported by the underyling code generator, extra validation is done in the middleware or handler layers instead. There's no guarantee that the code generator tells you it doesn't support a trait either. It might just silently ignore it.

For unmodeled members, the Smithy TypeScript and Rust server SDKs provide escape hatches for propagating unmodeled properties into the code-generated server stubs. The TypeScript server SDK has contexts while the Rust server SDK has extensions.

The data flows for the Smithy TypeScript and Rust server SDKs look like this today:

Smithy Server SDK Data Flow

Effectively, modeled inputs are generally restricted to what default deserialization can handle while everything else is generally placed in unmodeled inputs.

Context-styled escape hatches are fine if unmodeled information is a non-essential input to a service operation (e.g. observability) but introduces room for a lot of subtle bugs if the information is essential (e.g. caller identity). An extreme example is Go's context library which many misuse as an untyped map to pass both essential and non-essential information through their programs.

Bringing more info into Smithy models lets code generators enforce more complete modeled inputs before propagating them to middlewares and handlers.

Auth Example

To paint a clearer picture, lets consider auth.

For services with auth, operations are a function of the caller identity (e.g. deleteSong(userId, songId): deleteSongOutput if I destructure deleteSongInput into 2 arguments). Since identities are a formal resource in the service (e.g. AWS IAM identities like roles and users), it seems reasonable to model them like this in Smithy:

resource User {
    identifiers: {
        userId: string
    }
    properties: {
        // What other identities can do to this resource.
        resourceBasedPolicy: document
        // What this identity can do to other resources.
        identityBasedPolicy: document
    }
}

resource Song {
    identifiers: {
        songId: string
    }
    properties: {
        // What other identities can do to this resource.
        resourceBasedPolicy: document
    }
    delete: DeleteSong
}

@http(method: "DELETE", uri: "/song")
operation DeleteSong {
    input: DeleteSongInput
    output: DeleteSongOutput
}

@input
@references([
    {resource: User}
    {resource: Song}
])
structure DeleteSongInput {
    @required
    userId: String
    @required
    songId: String
}

@output
structure DeleteSongOutput {}

In some (many?) auth schemes, however, the caller identity may not be given as a field that should be provided as-is in code-generated server stubs. Instead, it may need to be pre-processed into a usable form.

For HTTP specifically, the Authorization header may not be something that's useful to directly map onto a member in modeled input types. In fact, Smithy strongly discourages this in the @httpHeader trait documentation.

For example, AWS service requests use the AWS SIGv4 authN scheme. The AWS SIGv4 HTTP Authentication header documentation provides this example header:

Authorization: AWS4-HMAC-SHA256 Credential=AKIAIOSFODNN7EXAMPLE/20130524/us-east-1/s3/aws4_request, SignedHeaders=host;range;x-amz-date, Signature=fe5f80f77d5fa3beca038a248ff027d0445342fe2855ddc963176630326f1024

An authZ middleware or business logic inside code-generated server stub implementations, however, probably instead only wants the AWS IAM unique ID (e.g. AKIAIOSFODNN7EXAMPLE) or an AWS IAM identity ARN (e.g. arn:aws:iam::111122223333:role/role-name) instead of the raw Authorization header.

Since the AWS IAM authZ APIs aren't public, lets use the Amazon Verified Permissions IsAuthorized API and AWS IAM Policy Simulator SimulateCustomPolicy API as examples. Both of these APIs generally are something like isAuthorized(callerIdentity, resource, operation, identityBasedPolicy, resourceBasedPolicy): boolean where callerIdentity isn't the raw Authorization header.

Likewise at the business logic layer, we may have a ListResources API which does a database query with a filter using a callerIdentity that isn't the raw Authorization header.

In essence, it can be useful to enforce some member transformation before finalizing modeled inputs for handoff to middlewares and business logic.

There's 2 ways I can think of doing this while keeping caller identity information in the modeled input with today's Smithy server SDKs.

Bind the Authorization HTTP Header Anyways + Use Mutating Middleware

One option is to ignore the @httpLabel guidance and bind the Authorization header anyways.

@input
@references([
    {resource: User}
    {resource: Song}
])
structure DeleteSongInput {
    @required
    @httpHeader("Authorization")
    userId: String
    @required
    songId: String
}

This might seem reasonable to do since the @authDefinition and @httpApiKeyAuth traits don't seem to let you map their caller identity info onto input shapes.

Instead, it seems like code generators need to add an additional argument to server stubs and clients that pipe caller identity information through (e.g. deleteSong(deleteSongInput, authInfo, context): deleteSongOutput) to avoid member name collisions in generated types.

AWS SDK code generators follow this style where caller identity information is separate from the modeled input. For example, in the AWS SDK for Java 2.x we can do:

import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider
import software.amazon.awssdk.services.lambda.LambdaClient

final var role_a_credentials = StaticCredentialsProvider.create(/* ... */);

final var role_b_credentials = StaticCredentialsProvider.create(/* ... */);

// Default to using role A credentials.
final var lambda_client = LambdaClient
    .builder()
    .credentialsProvider(role_a_credentials);
    .build();

final var role_a_invoke_response = lambda_client.invoke(request_builder -> request_builder
    .functionName("hello")
);

// Request-level override to use role B credentials.
final var role_b_invoke_response = lambda_client.invoke(request_builder -> request_builder
    .functionName("hello")
    // https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/awscore/AwsRequest.html
    .overrideConfiguration(config_builder -> config_builder
        .credentialsProvider(role_b_credentials)
    )
);

This is what the Smithy TypeScript server SDK expects people to do manually with contexts.

Then we introduce a mutating middleware that mutates the modeled input (e.g. DeleteSongInput) before passing it to subsequent middlewares and business logic.

Smithy Server SDK Data Flow - Mutating Middleware

No unmodeled inputs in this data flow diagram for simplicity.

This, however, introduces 2 subtle semantics.

First is that we've introduced an implicit dependency between middlewares because we're modifying the modeled input. The authZ middleware may not work if the caller identity mutation middleware isn't placed before it. If the server framework tries to run middlewares concurrently as a performance optimization, this middleware dependency information isn't automatically known to the framework. More abstractly, this approach has introduced mutability which allows a swath of bugs.

Second is that we've accidentally added a protocol-specific middleware in a place that should probably be protocol-agnostic. For a multi-protocol endpoint, other protocols may map a different value onto the modeled input member. The caller identity mutation middleware could get an HTTP Authorization header string or something else and must handle all cases correctly.

Use Contexts as a Side-Channel + Use Mutating Middleware

Another option is to just default to some empty/placeholder value for the modeled input member, add the value to a context object, then use a middleware to transfer the value from the context object to the modeled input.

@input
@references([
    {resource: User}
    {resource: Song}
])
structure DeleteSongInput {
    @required
    // This can get uglier if there's a @pattern trait on the referenced member.
    @default("")
    userId: String
    @required
    songId: String
}

Smithy Server SDK Data Flow - Transfer Middleware

If the context object is solely for capturing values to store in the modeled input, we can throw away the context afterwards like in the above diagram.

This is better than the other approach because the protocol-specific context creator can ensure the unmodeled input value it extracted is protocol-agnostic.

We still have the same mutation semantic though so we still have room for subtle ordering and concurrency bugs.

Wish List

Essentially, we'd like some standard trait respected across server code generators that lets users extend built-in deserialization in a safe and easy manner. This reduces the number of situations requiring less safe unmodeled input logic and middlewares that mutate modeled or unmodeled inputs.

Smithy Server SDK Data Flow - Extensible Deserialization

With a generic custom pre-processing trait, we could do something like this in our Smithy models:

use aws.protocols#restJson1
use aws.protocols#awsJson1_1

@input
@references([
    {resource: User}
    {resource: Song}
])
structure DeleteSongInput {
    @required
    @refine(functionId: "http_auth_SIGv4_deser", protocols: [restJson1, awsJson1_1])
    userId: String
    @required
    songId: String
}

A server stub code generator could then generate this:

public enum FunctionId {
    HTTP_AUTH_SIGV4_DESER="http_auth_SIGv4_deser"
}

public abstract class MusicServiceStub implements Service { /* ... */ }

public class SmithyHttpServer {
    // Used to check if all refiners were registered. Complain if not.
    private Set<FunctionId> expectedRefiners = Set.of(FunctionId.values());

    public void registerService(final Service service) { /* ... */ }

    public void registerRefiner(final FunctionId functionId, final Callable<SmithyHttpRequest> refinerFunction) { /* ... */ }

    public void start() { /* ... */ }
}

Users would then register their service and refiners with the HTTP server implementation.

public class MusicService extends MusicServiceStub { /* ... */ }

final var music_service = MusicService.create();

final var smithy_http_server = SmithyHttpServer.create();

smithy_http_server.registerService(music_service);

final var refiner = /* Create a refiner function. */;

smithy_http_server.registerRefiner(FunctionId.HTTP_AUTH_SIGV4_DESER, refiner);

smithy_http_server.start();

This is probably a horrible sample implementation for many reasons but hopefully it gets the idea across.

A more robust solution might look something like Java's JSR-330 dependency injection annotations (@Inject, @Named) and how Dagger 2 uses them. Dagger lets you define a dependency graph of value providers based on explicit @Named annotations and type inference. It then uses code generation at build time to create the dependency injection wiring logic.

Assumptions to Question

All the previous points are based on these premises/assumptions:

  1. Code generation can make modeled inputs safer than unmodeled inputs.
  2. Caller identity (auth) information should be part of modeled inputs instead of unmodeled/side-channel inputs.
  3. This trait as proposed is only expected to be respected by server code generators for doing raw request to modeled input deserialization.

1 is primarily an implementation detail so safety is highly dependent on how well the code generators are designed.

2 is probably where opinions will differ. Should all auth and caller identity related things be encapsulated in authentication traits and passed separately from modeled inputs? What effects does this have on the readability of Smithy models or client code generation (can we still easily pass caller identity information separate from modeled inputs like the current AWS SDKs)? Am I just splitting hairs about whether to put some info in one function argument versus another?

3 it's not clear how client generators are also expected to interpret this trait. It seems like it might be too server-centric though @clientOptional exists. Also if this is applied to an output shape, is this a signal to have a client side refiner for raw response to modeled output deserialization? In that case, the trait is anisotropic in a weird way that has different meanings between input and output shapes.

@commiterate commiterate changed the title Generic Custom Pre-Processing Traits & Caller Identity Modelling? Generic Custom Pre-Processing Traits & Caller Identity Modeling? Mar 11, 2024
@JordonPhillips
Copy link
Contributor

JordonPhillips commented Apr 23, 2024

Thanks for this detailed writeup!

[Reducing] the number of situations requiring less safe unmodeled input logic and middlewares that mutate modeled or unmodeled inputs

I think this actually dovetails into another problem we've been kicking around for a while that we're currently working on heavily: document schemas. Bear with me. You mention yourself that context is treated as an unmodeled bag of data, difficult to work with largely because you need to assemble the concrete representation yourself from it. This is essentially the same situation we find ourselves in with documents. Documents sometimes have defined schemas, and we therefore want a way to be able to easily construct concrete types from them.

I've actually got a working, hand-written example of this in Python. Here's an example from the tests:

def test_doc_to_shape():
    document = Document(
        {
            "birdId": "123456",
            "location": {
                "latitude": "52.5163",
                "longitude": "13.3777",
            },
            "notes": "Has black spots in the grey hood area.",
        }
    )
    shape = CreateSightingInput(
        bird_id="123456",
        location=Location(latitude="52.5163", longitude="13.3777"),
        notes="Has black spots in the grey hood area.",
    )

    assert document.as_shape(CreateSightingInput) == shape

So if I were using a Python server implementation, I could do something like:

@dataclass(kw_only=True)
class AuthContext:
    id: str

    def serialize(serializer: ShapeSerializer): ...

    def deserialize(deserializer: ShapeDeserializer): ...


class BirdWatcherService:
    async def create_sighting(input: CreateSightingInput, context: Document):
        id = context["auth"].as_shape(AuthContext).id
        ...

In this case the AuthContext class would be generated from a shape that looks like:

// Marked as internal so as to not be generated into clients.
@internal
structure AuthContext {
    id: String
}

What's critical here is that that deserialize method is the same method we would use to deserialize the shape if we were instead parsing from a JSON or XML formatted byte stream over HTTP. So any constraints, validation, or other logic would carry over. This is also true for serialize.

You still of course need your middleware to take from the input, perform any necessary validations / transformations, and then populate the context. But with this you would be able to ensure that your context is consistently structured:

async def auth_middleware(
    request: HTTPRequest,
    context: Document,
    next: Callable[[HTTPRequest, Document], Any]
) -> None:
    id: str = await validate_auth(request.headers["Authorization"])
    context["auth"] = Document.from_shape(AuthContext(id=id))
    next(request, context)

All this of course relies on the ability to have shapes in the model not connected to an operation, which is something we will be adding. It also requires a pretty hefty refactor of the way clients and servers work, but that's also something we want to do where possible. I'm working on this right now for smithy-python, which has the advantage of not having been made GA yet.

To address some of your assumptions:

  1. Code generation can make modeled inputs safer than unmodeled inputs.

Essentially what I've outlined above is a strategy to bring any-type data into a similar realm of reliability as normally structured data.i

  1. Should all auth and caller identity related things be encapsulated in authentication traits and passed separately from modeled inputs?

Yes. Auth is expected to change, and to do so as backwards compatibly as possible. Having to change the code for every operation and operation invocation would be a major hassle. Comparatively it's often possible to simply smooth over the config changes on the client side. On the server side, the middleware can similarly present the necessary information (e.g. customer id) in a consistent way despite the auth type having different inputs.

  1. This trait as proposed is only expected to be respected by server code generators for doing raw request to modeled input deserialization.

With the strategy outlined above, you don't need a trait, just some shapes. They can be made internal or otherwise be tagged to filter out during client code generation.

@commiterate
Copy link
Author

commiterate commented Apr 26, 2024

Just to clarify my understanding, it sounds like the hope is to:

  1. Have code generators generate protocol-agnostic types/classes/specs/whatever for shapes not tied to an operation (in the auth case, some shape representing the caller identity like MyCallerStruct).
  2. Have users use those types in their context objects (e.g. context.caller: MyCallerStruct).
  3. Have users write the protocol-specific de/serialization bits which translate between protocol-specific types and protocol-agnostic types (i.e. figures out how to de/serialize context.caller).
    • Since it's manual, we can do whatever crazy stuff we want like transforming AWS SIGv4 HTTP headers into an AWS IAM identity struct.
    • This is added either before the modeled input deserialization logic (what the TypeScript and Rust server SDKs seem to prefer because raw protocol-specific payloads like the full HTTP request are available here) or after it (the Rust server SDK calls this an operation-specific model middleware. Raw protocol-specific requests are not visible here).

I think this covers the case of stuff that should stay outside of modeled inputs (i.e. in contexts) yet still be typed. These are now partially modeled, but the Smithy model itself gives no hints as to what inputs or outputs they complement.

This, however, still leaves us with modeled inputs being restricted to what default deserialization can handle (i.e. basically 1:1 payload field to struct field de/serialization or validation. Can't map multiple payload fields into a single struct field or complex protocol-specific field transformations). The built-in default deserialization that comes with a Smithy server SDK (and what I imagine most people will stick with) still feels closed/non-extensible instead of open/extensible.

Suppose a Smithy Java server code generator can auto-generate routing and modeled input de/serialization code for Apache Tomcat as the HTTP server (may sound familiar to Builder Tools). If we want to add a small transformation for a specific field, we have to completely replace the default deserializer with our own or use a mutating middleware instead of just registering a small function the default deserializer can defer to when handling this special field.

If we go heavy on the Java dependency injection paradigm, I was imagining something like this (with totally illegal syntax for compactness, not that this is very compact):

import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import java.nio.charset.StandardCharsets;

// Generated by Smithy Java server code generator.
class MyInput {
    @Inject
    @Named("MyInput.fieldA")
    final String fieldA;

    @Inject
    @Named("MyInput.fieldB")
    final int fieldB;
}

// Generated by Smithy Java server code generator.
abstract class SmithyDefaultMyInputRestJson1Providers {
    private final ObjectMapper objectMapper = new ObjectMapper();

    // Request body is a UTF-8 JSON string. Make it a dynamic tree first.
    //
    // Something like a Jackson JsonNode that can represent trees
    // of basic primitive (e.g. boolean, int, string) or collection types
    // (e.g. map, list) that most serialization formats support.
    //
    // Smithy's document type is kind of this already, so the Smithy Java
    // server SDK could implement something with similar behavior. Just using
    // Jackson JsonNode for convenience here.
    //
    // Dynamically typed languages can use the language-native collection
    // types instead like Python (dict, list) or Clojure (map, vector).
    //
    // For example, Python would use json.loads() and browse a dict.
    // Clojure would use clojure.data.json/read-str and browse a map.
    //
    // Statically typed languages would have an extra step that maps
    // these generic nested collections onto a nominal type (since most
    // statically typed languages besides OCaml, Scala, and TypeScript
    // don't support structural types).
    //
    // This 2-shot deserialization process that first translates to
    // generic collections provides hooks for custom field-level
    // deserialization in an ergonomic manner.
    @Provides
    @Singleton
    @Named("jsonBody")
    @Protocols("aws.restJson1")
    JsonNode provideJsonBody(final SmithyHttpRequest request) {
        return objectMapper.readTree(
            StandardCharsets.UTF_8.decode(request.body()).toString());
    }

    // Default field A deserializer.
    @Provides
    @Singleton
    @Named("MyInput.fieldA")
    @Protocols("aws.restJson1")
    String provideFieldA(@Named("jsonBody") final JsonNode jsonBody) {
        return jsonBody.findPath("fieldA").asText();
    }

    // Default field B deserializer. This has the custom deserialization trait
    // on the Smithy model, so the user must provide the implementation.
    @Provides
    @Singleton
    @Named("MyInput.fieldB")
    @Protocols("aws.restJson1")
    abstract int provideFieldB(@Named("jsonBody") final JsonNode jsonBody);
}

class CustomMyInputRestJson1Providers extends SmithyDefaultMyInputRestJson1Providers {
    // Use default deserializer for field A.
    // The Smithy Java server framework's default deserializer is good enough here.

    // Provide custom deserializer for field B.
    @Provides
    @Singleton
    @Named("MyInput.fieldB")
    @Protocols("aws.restJson1")
    int providefieldB(@Named("jsonBody") final JsonNode jsonBody) {
        final int originalFieldB = jsonBody.findPath("fieldB").asInt();

        return originalFieldB * 100;
    }
}

Side note: For Java, we can still use regular collections by just doing things like List<Object> or Map<Object, Object> and using instanceof pattern matching on types. This requires using the object version of primitives (e.g. int v. Integer).

Any validation logic can be applied after these field-level deserializers are run to generate the composite structure.

Smithy traits already inherently have an extract-transform-load (ETL) ordering to them. For example, @jsonName (extract) being applied before @length ("transform"/validate) even if they are declared in a different order in the model. Essentially, each field is limited to 1 de/serialization trait per protocol and unlimited validation traits (validation operations are commutative).

In that sense, maybe this single trait I'm proposing is actually 3:

  1. @deserializer(functionId: String | Enum, protocols: List[shapeId])
    • Protocol-specific deserializer.
    • Only 1 can be applied to a field per protocol (mutually exclusive).
  2. @serializer(functionId: String | Enum, protocols: List[shapeId])
    • Protocol-specific serializer.
    • Only 1 can be applied to a field per protocol (mutually exclusive).
    • We could combine this with @deserializer into something like @mapper/@isomorphism and require users to provide both the deserializer and serializer implementations.
  3. @validator(functionId: String | Enum)
    • Protocol-agnostic validator.
    • Infinitely many can be applied to a field.

The existing de/serialization, constraint, and type refinement traits are just special cases of these.


As for the maintenance benefit of leaving auth outside of modeled inputs, this one isn't as clear to me. Is the concern having to manually maintain caller identity members and traits on every operation's input shape? For example:

resource User {
    identifiers: {
        userId: string
    }
    // ...
}

@input
@references([
    {resource: User}
])
structure Operation_1_Input {
    @required
    userId: String
    // ...
}

// ...

// We've repeated @references, @required, and the field 100 times now.
@input
@references([
    {resource: User}
])
structure Operation_100_Input {
    @required
    userId: String
    // ...
}

If we, for example, need to add a trait like @pattern, we'd have to add it to all operations. This seems like something that can be dealt with using a dedicated caller identity structure and/or a mixin. For example:

// --------------------
// Structure approach.
// --------------------

@references([
    {resource: User}
])
structure CallingUser {
    @required
    // Any other traits.
    userId: String
}

@input
structure Operation_1_Input {
    @required
    callingUser: CallingUser
    // ...
}

// --------------------
// Mixin approach.
// --------------------

@mixin
@references([
    {resource: User}
])
structure BaseInputAuth {
    // This is a bit nasty if certain operations have optional auth.
    @required
    // Any other traits.
    userId: String
}

@input
// Might cause problems if the operation input shape also applies @references
// directly since, according to the mixin docs, that takes precedence over
// traits inherited from a mixin.
//
// Even if it didn't get overwritten and just became multiple mixin applications,
// multiple declarations of the same mixin doesn't follow a last-writer-wins
// or union behavior. Last-writer-wins is particularly undesirable since mixin
// application order matters then.
@references([
    {resource: User}
])
structure Operation_1_Input with [BaseInputAuth] {
    // ...
}

I can still see some complaints about this being more verbose than desirable. The @auth and @optionalAuth traits already give operation-level auth opt-out control.

Basically, it's whether auth should be opt-out by default (auth trait on a service to swap to opt-in by default, @auth and @optionalAuth traits on operations for opt-out) or opt-in by default (use a shared structure or mixin to add auth).

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

No branches or pull requests

2 participants