Warn rather than raise on opportunistic auth failure.

[twosigmajab]

When opportunistic_auth is enabled but an Authorization header could not be generated opportunistically, log a warning rather than raising an exception. (If the request results in a 401 and we fail to generate an Authorization header in that (no-
longer-opportunistic) case, an exception is still raised.)

Motivation: We'd like to enable opportunistic auth by default for requests to internal services within our network, but would not be able to easily flip that switch without these changes for $REASONS. (There are a number of existing integration tests that run under network isolation where the client makes a request to a server running locally that doesn't actually require auth. Making opportunistic auth no longer a hard failure would allow us to default all clients to opportunistic auth without such tests failing.)

This behavior also matches a reasonable interpretation of the word "opportunistic": try early, but if that fails, don't just give up.

---

UPDATE: The following changes will be split out from this PR into a separate PR

While I was making the above changes, I also noticed some issues with the logging in this code, so I fixed them while I was in here:

• Never log Authorization header values to prevent unintentional leaking of credentials.
• Consistently use logging's built-in string interpolation everywhere, rather than using wasteful str.format() in some places. Using str.format() in logging calls is an anti-pattern, because when the log message would be filtered out anyway (e.g. when the level is too low), it wastes time on string interpolation results that are never used.
• Don't make two different logging calls, one after another, for the same event. Combining them into a single logging call is more efficient and interoperable with non-line-based logging formatters and aggregators (e.g. JSON and Datadog, etc.).

[frozencemetery]

Please refer to https://chris.beams.io/posts/git-commit/ for how to style commit messages. Additionally, each proposed change should be its own commit. I am happy to re-review when you have something more presentable.

[frozencemetery]

This is not how this file is used. Please refer to git history, or look at the previous entries.

[twosigmajab]

Thanks for your response. I am happy to make the requested changes.

But in the meantime, the bottom 20 lines of this patch already include the isolated changes for "warn rather than raise on opportunistic auth failure". Here is a link with just those 20 lines highlighted:

https://github.com/pythongssapi/requests-gssapi/pull/31/files#diff-b6ed09d14ac055cc2aa676d962e97e5b63bd057b3f25a688b31c1b1b7cbc6d8eL309-R319

Would it be possible to say whether you would consider accepting a patch that accomplished what is proposed in the title of this PR based solely on the rationale given in its description? And if it would help to have a reference implementation, you can look at only the 20 lines I've highlighted?

[frozencemetery]

I am willing to consider many things :)

Concretely, I'm not opposed to the fallback-type behavior. I'm undecided on whether hijacking the existing opportunistic=True behavior is the right way to go on that: while I agree it's unfortunately named, adding a fallback path may result in a second request in the failure case. I see three possibilities for that:

1. It's fine; no one will care about an error log message when it's not going to work anyway.
2. requests-kerberos strict compatibility is important, so make the semantic change only when we're not shimming.
3. Turn the boolean into a three-state: true, false, and "fallback" (or so).

It seems like you're more inclined to (1), right? That's probably okay but we need to take some care that we only retry when the server wasn't prepared, not for general auth failures.

[twosigmajab]

Thank you, great to know you're open to this!

Yes, I'm more inclined to (1).

Regarding compatibility with requests-kerberos, @geofft and I recently became maintainers of it, and would be interested in making similar changes there if there are no concerns from the community. So far it sounds like we can coordinate a convergent path for this, which is great to hear.

I've removed the unrelated changes in the latest revision and updated the commit message to conform to the style guide you're using.

we need to take some care that we only retry when the server wasn't prepared, not for general auth failures.

Yes, as the updated commit message hopefully now makes clearer, that is the intended effect of these changes.

Please let me know if this looks good to merge now, or if any other changes are in order.

Thanks again for your consideration.

[frozencemetery]

Regarding compatibility with requests-kerberos, @geofft and I recently became maintainers of it, and would be interested in making similar changes there if there are no concerns from the community. So far it sounds like we can coordinate a convergent path for this, which is great to hear.

Very intriguing! At some point we should talk about how to fix #8 :)

Please let me know if this looks good to merge now, or if any other changes are in order.

I fixed a couple nits. If it still looks good to you, I'm ready to merge it.

[twosigmajab]

Fantastic!

Looks like the latest revision introduces logging calls that exhibit some of the problems I originally flagged (still listed in the description above). But happy to get this merged as-is and take the logging concerns to a separate issue.

I'd have to catch up on #8 (and related discussion in requests/requests-kerberos#133 etc.), but @geofft may have more context on that. In any case, I'm sure we'd love to see that addressed in both projects, and would be happy to coordinate further on that whenever we can.

Thanks for merging this!

[frozencemetery]

Okay. Please do follow up on the logging concerns if you're still interested in them - I think we'll need a bit more discussion but I'm sure we can make something work :)