feat: Add --oauth-force-oob CLI option

[laurentsimon]

Adresses comment #665 (comment)

[woodruffw]

Thanks for adding this! A couple of nitpicks 🙂

[woodruffw]

I think this flag name might be a little ambiguous: what it's doing is forcing the OOB flow, which is conceptually untied to the "default" browser. It's also doing an OAuth2 flow, so I think we should emphasize that rather than OIDC (OIDC refers to the "shape" of the claims that come out of the flow).

What do you think about --oauth-no-browser or --oauth-force-oob?

[woodruffw]

(I have a slight preference for the latter, since it'll be consistent with the environment suggestion below. But either is fine IMO; your call.)

[laurentsimon]

I renamed --oauth-force-oob, even though I think prepending --oidc-xxx wold be more consistent with other commands if it's under the oidc group. Lmk

[woodruffw]

Yeah, it's a little inconsistent, but IMO it's worth it for the compatibility/symmetry with the existing flag.

There might be a better section to put this under; I'll take a look.

[woodruffw]

JFYI: This will also require a CHANGELOG.md entry and updates to the --help outputs in the README.md. But we can do that as the last step.

[woodruffw]

/gcbrun

[woodruffw]

LGTM behaviorally!

check-readme is unhappy, but I'll fix that:

# sigstore --help
# sigstore sign --help
4,5c4,5
<                      [--no-default-files] [--signature FILE]
make: *** [Makefile:[12](https://github.com/sigstore/sigstore-python/actions/runs/5158329157/jobs/9292054589?pr=667#step:5:13)3: check-readme] Error 1
<                      [--certificate FILE] [--bundle FILE]
---
>                      [--oauth-force-oob] [--no-default-files]
>                      [--signature FILE] [--certificate FILE] [--bundle FILE]
30c30,32
<   --oauth-force-oob     Force an out-of-band OAuth flow and do not automatically start the default web browser (default: False)
---
>   --oauth-force-oob     Force an out-of-band OAuth flow and do not
>                         automatically start the default web browser (default:
>                         False)

[woodruffw]

/gcbrun

[woodruffw]

LGTM, thanks @laurentsimon!

I need one of the other maintainers to approve as well, but this will be included in the 2.0 series.

[woodruffw]

/gcbrun

[woodruffw]

/gcbrun

[woodruffw]

/gcbrun

[woodruffw]

/gcbrun

[woodruffw]

I have no clue why the macOS and Windows CIs are getting stuck here. Maybe some kind of bad runner state? Nothing in this PR itself should vary between platforms.

[woodruffw]

Weirdly, both seem to hang at around the same test:

Windows:

2023-06-08T20:15:13.5528997Z test/unit/internal/oidc/test_issuer.py::test_fail_init_url PASSED        [ 75%]
2023-06-08T20:15:13.6085787Z test/unit/internal/oidc/test_issuer.py::test_init_url PASSED             [ 75%]
2023-06-08T20:22:23.5095829Z ##[error]The operation was canceled.
2023-06-08T20:22:23.6691779Z Post job cleanup.
2023-06-08T20:22:24.0268157Z [command]"C:\Program Files\Git\bin\git.exe" version

macOS:

2023-06-08T20:15:08.1732720Z test/unit/internal/oidc/test_issuer.py::test_fail_init_url PASSED        [ 75%]
2023-06-08T20:15:08.1998280Z test/unit/internal/oidc/test_issuer.py::test_init_url PASSED             [ 75%]
2023-06-08T20:22:31.5320710Z ##[error]The operation was canceled.
2023-06-08T20:22:31.5554660Z Post job cleanup.
2023-06-08T20:22:32.1636730Z [command]/usr/local/bin/git version
2023-06-08T20:22:32.2323580Z git version 2.40.1

I'll do some more digging later.

[di]

I wonder if these are waiting for user input... perhaps we need to add a reasonable timeout around any flows that wait for user input?

[woodruffw]

I wonder if these are waiting for user input... perhaps we need to add a reasonable timeout around any flows that wait for user input?

Yeah, that was my guess, although it's a mystery to me why they'd block on these platforms and not on the Linux CI.

I think a timeout around user input makes sense, in places we can enforce it (e.g. stdin and not the browser) -- I'll look into that as well.

[woodruffw]

This was indeed a silly blocking thing; should be fixed with d6052c9.

[woodruffw]

/gcbrun

[di]

How was this passing on linux? E.g. https://github.com/sigstore/sigstore-python/actions/runs/5204665487/jobs/9412771815

[woodruffw]

I think it probably passed because we're not being specific enough about which IdentityError is being caught in the test.

Looking at it more closely, I'm not 100% sure what branch this test was originally intended to cover -- @jleightcap do you remember what your intent with this was?

[woodruffw]

It looks like it was probably meant to catch this:

sigstore-python/sigstore/oidc.py

Lines 361 to 364 in d610255

	try:
	resp.raise_for_status()
	except requests.HTTPError as http_error:
	raise IdentityError from http_error

I'll add a proper exception message there and assert on it. My guess is that the Linux CI was passing because webbrowser.open() failed on it and pushed it into the OOB flow, even without force_oob.

[woodruffw]

/gcbrun

[woodruffw]

Tests are passing with the new match, so that confirms that the Linux CI was coincidentally entering the OOB flow (whereas Windows and macOS weren't, probably because both have a browser installed).