Skip to content

tests: Revise OAuth2 tests#3795

Merged
polarathene merged 11 commits intodocker-mailserver:masterfrom
polarathene:tests/auth-revise-oauth2
Jan 20, 2024
Merged

tests: Revise OAuth2 tests#3795
polarathene merged 11 commits intodocker-mailserver:masterfrom
polarathene:tests/auth-revise-oauth2

Conversation

@polarathene
Copy link
Copy Markdown
Member

@polarathene polarathene commented Jan 19, 2024

Description

This is a follow up to the original feature PR #3480

Each commit provides commit message for extra context. Changes have been staged into iterations towards the full review diff (so there's a little bit of extra noise there). Should be easy to follow if the full diff is too much.

Overview

  • Replaced the Python mock service with Caddy, it'll be used in other tests such as for MTA-STS in future.
  • Extended test coverage to both SASL mechanisms the feature enables:
    • XOAUTH2 (Googles widely adopted implementation) and OAUTHBEARER (the newer variant standardized by RFC 7628 in 2015).
    • Both are supported as different ways to verify authorization against the identity provider via the same OAuth Bearer Token (RFC 6750, 2012).
  • Better asserts in the test-cases, and more thorough / accurate documentation for maintainers (I could probably improve the Caddy specific information, but I think it's roughly intuitive enough).
  • The Caddy service additionally provides an endpoint that handles generation of the credentials base64 encoded.
    • Similar to what the Python script did except you can use curl against it with the two query string parameters for dynamic values.
    • This may be useful in future, but is simple to use with the complimentary compose.yaml for troubleshooting outside of a running test.

If time would permit, I'd extend the tests further for Dovecot configuration related to this feature. Caddy keeps the interactions fairly simple and transparent for communicating expectations to maintainers along with assistance in troubleshooting.

Extra info

If needing to troubleshoot, adding the following (global options) to the Caddyfile for more helpful information:

{
  # Enables logging without omitting sensitive data
  # https://caddyserver.com/docs/caddyfile/directives/log
  # https://caddyserver.com/docs/caddyfile/options#log-credentials
  servers {
    log_credentials
  }

  # Slightly better output for console viewing:
  # https://caddyserver.com/docs/caddyfile/directives/log#format-modules
  log {
    format console {
      # This is a Go time format string, these values are not random but intentional:
      time_format "15:04:05"
    }
  }
}

Type of change

  • Improvement (non-breaking change that does improve existing functionality)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • If necessary I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have added information about changes made in this PR to CHANGELOG.md

Better documented, easier flow and separation of concerns via Caddy.

The python code had additional noise related to setting up a basic API which is abstracted away via `Caddyfile` config that's dedicated to this task.
Caddyfile can use an Access Token instead of a JWT. Much smaller and correct for this OAuth2 configuration. This new value has been documented inline.

Likewise the `sub` field returned is not important to this test. `email_verified` is kept as it may be helpful for further coverage testing.

The actual test-case has better assertions for success and failure by checking for Dovecot logs we expect instead of netcat response.

`oauth2` to `auth` for the Caddy container hostname is not necessary, just a more generic subdomain choice.
This way is more flexible and doesn't require modifying the `Caddyfile` directly, while still easy to use.

Additionally simplifies understanding the Caddyfile to maintainers by removing the `route` directive that was required to ensure a deterministic order of vars.
Since this is the only intended usage, might as well have it respond with the full file content.
Caddyfile route for `/imap/` now accepts any subpath to support handling both `xoauth2` and `oauthbearer` subpaths.

Both SASL mechanisms represent the same information, with `XOAUTH2` being a common mechanism to encounter defined by Google, whilst `OAUTHBEARER` is the newer variant standardized by RFC 7628 but not yet as widely adopted.

The request to `/userinfo` endpoint will be the same, only the `credentials` value to be encoded differs.

Instead of repeating the block for a similar route, this difference is handled via the Caddyfile `map` directive.

We match the path context (_`/xoauth2` or `/oauthbearer`, the `/imap` prefix was stripped by `handle_path` earlier_), when there is a valid match, `sasl_mechanism` and `credentials` map vars are created and assigned to be referenced by the later `respond` directive.

---

Repeat the same test-case logic, DRY with log asserts extracted to a common function call. This should be fine as the auth method will be sufficient to match against or a common failure caught.
Separate test cases and additional comment on creating the same base64 encoded credentials via CLI as an alternative to running Caddy.

Added a simple `compose.yaml` for troubleshooting or running the container for the `/imap/xoauth2` / `/imap/oauthbearer` endpoints.
`reverse_proxy` was a bit more convenient, but the additional internal ports weren't really relevant. It also added noise to logging when troubleshooting.

The `import` directive with Snippet blocks instead is a bit cleaner, but when used in a single file snippets must be defined prior to referencing them with the `import` directive.

---

`compose.yaml` inlines the examples, with slight modification to `localhost:80`, since the Caddyfile examples `auth.example.test` is more relevant to the tests which can use it, and not applicable to troubleshooting locally outside of tests.
@polarathene polarathene added service/dovecot area/tests kind/improvement Improve an existing feature, configuration file or the documentation labels Jan 19, 2024
@polarathene polarathene added this to the v13.3.0 milestone Jan 19, 2024
@polarathene polarathene self-assigned this Jan 19, 2024
Comment thread test/config/oauth2/Caddyfile
Copy link
Copy Markdown
Member

@georglauterbach georglauterbach left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

I am not too deep into OAUTH2, but might use it as well (later) when we have something like OIDC going. These changes look very good, and the additional information in the form of comments is appreciated!

Comment thread test/tests/serial/mail_with_oauth2.bats Outdated
Comment thread test/tests/serial/mail_with_oauth2.bats
casperklein
casperklein previously approved these changes Jan 19, 2024
@polarathene polarathene enabled auto-merge (squash) January 20, 2024 08:49
@polarathene polarathene merged commit f3a7f08 into docker-mailserver:master Jan 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/tests kind/improvement Improve an existing feature, configuration file or the documentation service/dovecot

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants