fsprojects / FsHttp

A lightweight F# HTTP library by @SchlenkR and @dawedawe
https://fsprojects.github.io/FsHttp/
Apache License 2.0
424 stars 42 forks source link

How to strip secrets from requests and responses? #195

Closed greggyb closed 1 month ago

greggyb commented 1 month ago

I really like this library and appreciate the hard work and thoughtful design that have gone into it.

I find myself running into a recurring issue. It is cumbersome to add auth headers at the last minute to arbitrary Requests and to strip these out of a Response.

In my primary use case, I've got an Auth module that is responsible for getting and renewing auth tokens. This is managed entirely in the module and the data is kept private there. The interface to the rest of the program consists of Auth.configure which takes a configuration and sets up the tokens (and configures refresh of the same, and manages this lifetime) and Auth.authAndSend, which takes a request, adds an auth header, sends the request, and strips the auth header out of the response.

This gets cumbersome because the output of http { ... } is not uniform. I can get a HeaderContext, a BodyContext or a MultipartContext in my use cases. If a header context, I can easily add an auth header. For body and multipart contexts, it seems to me that I have to reach into the fields and modify header.headers myself, which means I need to know whenever I use the CE what the output is and wrap it in a discriminated union that I can match on later to understand how to construct the new context with auth header. (I could also use an active pattern, but still not ideal).

Then I have to strip the token out of the response. Response has multiple copies of the original request, both FsHttp's domain type and a few fields that expose System.Net.Http.HttpRequestMessage.Headers. So, I strip these out of the response before passing it back out of my Auth.authAndSend function.

It feels like I am working against the library to do this. Am I missing some other approach? Especially with regard to adding auth at the very end. I would love to be able to pass around partial requests, but this seems only to work for body-less requests.

greggyb commented 1 month ago

For reference, I was able to modify my approach to centralize most of the logic into a redaction function and some active patterns.

There is one place that calls into this authAndSend in a trusted and heavily audited section of code that deals with token acquisition and renewal. That call-site does not expose the token, but just passes it into this authAndSend.

I dislike this less now that I've refactored (I had more wrappers before and was using a DU to match on in authAndSend, but that required all locations where I built an http request would assign the correct union type).

Is there a better way to go about this?

    let inline private redactAuth response =
        let req = response.request

        let scrubbedReqHeader =
            req.header.headers |> Map.mapValue "Authorization" redactBearer // just string manipulation factored out

        let scrubbedReq =
            { req with
                header.headers = scrubbedReqHeader }

        response.requestMessage.Headers.Remove "Authorization" |> ignore

        { response with request = scrubbedReq }

    let inline (|Header|_|) requestContext =
        match box requestContext with
        | :? HeaderContext -> Some requestContext
        | _ -> None

    let inline (|Body|_|) requestContext =
        match box requestContext with
        | :? BodyContext -> Some requestContext
        | _ -> None

    let inline (|Multipart|_|) requestContext =
        match box requestContext with
        | :? MultipartContext -> Some requestContext
        | _ -> None

    let inline authAndSend requestContext token =
        let addToken = Map.add "Authorization" <| sprintf "Bearer %s" token

        let authorized =
            match box requestContext with
            | Header h -> (h :?> HeaderContext) { AuthorizationBearer token } |> Request.sendAsync
            | Body b ->
                let bb = b :?> BodyContext

                { bb with
                    header.headers = addToken bb.header.headers }
                |> Request.sendAsync
            | Multipart m ->
                let mm = m :?> MultipartContext

                { mm with
                    header.headers = addToken mm.header.headers }
                |> Request.sendAsync
            //TODO: improve error
            | _ -> failwith "should never happen"

        authorized |> Async.map redactAuth
SchlenkR commented 1 month ago

Hi and thanks for your thoughts.

I do not completely understand your solution for your use case. Let me answer in a more general way: As you have already figured out, the Request-types are built in a way that all parts (or sections) of the whole request must retain an order. So it is indeed not possible dealing with these parts as if they were one. The only exception is the URL (this was introduced in v14.5) with the goal of having better composability. Basically, that's it; it's the way the library works. If your "workarounds" (maybe not even a workaround) is ok - then it's ok :)

Apart from that, it could be interesting to figure out: Why are you relying on the FsHttp model, and later trying to extract information from it, even though that's not always possible? Could be that it's not the appropriate choice of representation at that piece of code. But I can't really evaluate that.

I would close this issue due to no further actions to be carried out here.

Thank you!

greggyb commented 1 month ago

Thanks for the response! I appreciate it, and recognize that this is not exactly actionable as-is. I was mostly hoping for feedback or some more obvious solution I'd overlooked. It sounds like I haven't missed anything like that.

Why are you relying on the FsHttp model, and later trying to extract information from it, even though that's not always possible?

The FsHttp model maps to my use cases very well, except for this question I raised. I build up FsHttp request contexts incrementally, and pass around these partially configured items as part of the normal course of operation, and the CE DSL works very well for that.

I just don't want to be passing around bearer tokens throughout the entire request lifecycle. I want to minimize parts of my code-base that have knowledge of such secrets. The code I shared above does the core work of adding an auth token to an arbitrary, otherwise-complete, request context and then removing the same auth header from responses. In this way I can freely log the entire response domain object when running under debug mode, or when there are errors. This is super useful when troubleshooting the application. I can log an entire request context or an entire response without ever worrying that auth tokens land on disk somewhere. The one place I need to be careful about that is in the middle of my authAndSend I shared above.

Again, no action necessary and this is fine as closed. I just wanted to make sure I wasn't missing a library capability that would make my workflow better. It sounds like no, so I'll continue with the approach I shared above.

Thanks again for an incredibly useful library!