(CODEMGMT-1110) Allow specifying a forge token in config

[Magisus]

This commit adds a new config value to the forge section,
authorization_token, that can be used to supply a token to be used to
contact a private Puppet Forge instance. It will supersede the token
from the PE license, if that exists.

This mirrors what we do in Puppet: setting. But open to suggestions, e.g. if we want this to be a file path like it is for the OAuth token, instead.

[Magisus]

I haven't been able to test this against a real forge yet, but would appreciate review on the approach.

[justinstoller]

Seems like a good approach.

[mwaggett]

yeah, this approach seems good to me.

Our forge doesn't require authentication, right? Do we want to ensure that the setting to use an alternate forge is set if authorization_token is set?

[mwaggett]

also not sure about file vs. putting the token directly in the yaml file.. the latter does seem probably less secure lol

[Magisus]

Do we want to ensure that the setting to use an alternate forge is set if authorization_token is set?

Not sure, that might be a good idea?

I'll try to see if I can track down any input from the people asking for this, for either of those things.

[Magisus]

Talked to Nick, he's fine with this as a starting point, with the token directly in the config (since we're not doing a CLI flag for this). He's going to reach out to customers and see if they also want support for username+password, but this is sufficient to start with.

[Magisus]

Alright, this works if you precede the token with Bearer. Do we think that should be something we should document to include in the config if you're using Artifactory, or should we append it automatically? Artifactory seems to require it, but I don't know if other APIs would. See "Authorization Headers" here. Puppet does not include it: to authenticate against my artifactory with puppet module install, I had to add the Bearer to the front of the forge_authorization value in puppet.conf.

I'm leaning towards making that something users are responsible for.

[mwaggett]

I think it makes sense to just document the need for Bearer, both because we don't know if other APIs require it and so that we're consistent with puppet.

I don't suppose it would be possible/easy to write an automated test of the desired behavior? Or do we think that's not worthwhile?

[Magisus]

There's state bleeding from the first new test apparently... not sure how though...

[Magisus]

If you're talking like a beaker test, I don't think that will really be feasible/worth it, given what it takes to set up a fake private forge.

[mwaggett]

I didn't necessarily mean beaker, but if any automated test would have to set up its own Forge (as opposed to using the one you set up already??), then yeah probably not worth it.

[mwaggett]

hmm, do we need to call setup_settings again to fix the state after your tests?

[Magisus]

The one I set up lives on a pooler node. I'm not loving the idea of maintaining a permanent Artifactory instance just to test this, especially considering you need the pro license to mimic the Forge.

[Magisus]

In looking into these tests, I found this https://github.com/puppetlabs/r10k/blob/main/lib/r10k/initializers.rb#L63-L66. Not sure if I should try to rework this to fit it into that somehow... might give it a shot.

[mwaggett]

yeah probably not worth it if it's hard, just wasn't sure if the thing you set up could happily like within our existing Artifactory instance that RE maintains.

I don't really know what those initializers are used for, but, at least for consistency, it makes sense to me to add something there for the auth token.

[Magisus]

Using those is the right way to do this. I also figured out where the state bleed is coming from (PuppetForge.host and PuppetForge::Connection.authorization are class variables, setting them needs stubbing).

[Magisus]

Maybe this is something to bring up for the folks that are trying to replace the dev forge?

[Magisus]

Tested this out again with my Artifactory install, seems to work well!

[mwaggett]

@justinstoller I believe your PR bumped this to 9.2.10. Any reason not to go all the way to 9.2.17?

[justinstoller]

I put it to 9.2.10.0 in my PR because that version should be the oldest that works with the forge changes

[justinstoller]

FWIW, when documenting this and calling out the necessity of the "Bearer " portion of the token string it may be valuable to link how the Forge itself is: https://forgeapi.puppet.com/#section/Authentication and that this is common to OAuth2 https://datatracker.ietf.org/doc/html/rfc6750 while the general standards for Authorization tokens do leave the <type> field wide open and is currently beyond our intention to implement: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Authorization