Add secret-less authentication for CI jobs. #131

plietar · 2024-09-20T12:51:18Z

This change allows automated environments, such as Github Actions or Buildkite to authenticate to the Packit API without needing to be manually provisioned with a secret.

The client's environment creates and signs a JWT containing claims describing the job. For example, on Github Actions, GitHub provides a token which includes the organisation name, repository name, type of event, branch name, etc. Packit needs to be configured to trust the 3rd party JWT issuer, with policies specifying what claims are needed.

The client sends the JWT it received from its environment to the Packit /auth/login/jwt endpoint, and in exchange receives a Packit JWT which may be used to access the rest of the API.

plietar · 2024-09-24T16:46:21Z

api/app/src/main/kotlin/packit/config/JwtLoginConfig.kt

+  val tokenDuration: Duration? = null,
+)
+
+@ConfigurationProperties(prefix = "auth.external-jwt")


This deviates from our current approach we've been using in AppConfig. I needed to do this because of the more complex structure the policies has (it is a list of a class which includes a map and a list as sub-fields).

Eventually I think it would actually be nice to move all configuration to this way of doing.

do we need to add these into the applcicaton.properties? like the ones described in auth.md docs as well?

Suggested change

@ConfigurationProperties(prefix = "auth.external-jwt")

@ConfigurationProperties(prefix = "auth.externalJwt")

as per docs? or does it hyphen the camelcase?

do we need to add these into the applcicaton.properties

Because these are much more structured, I don't think it makes sense to use flat string environment variables to pass them in, but instead need to pass them as structured data.

There's a number of ways we can do it, see: https://docs.spring.io/spring-boot/reference/features/external-config.html

as per docs? or does it hyphen the camelcase?

So Spring is kind of weird and I got errors here if I didn't use kebab case here. It seems to apply some normalization to the input properties, which means that it doesn't actually matter what case people use, and both external-jwt and externalJwt would work.

See https://github.com/spring-projects/spring-boot/wiki/Canonical-properties
And https://github.com/spring-projects/spring-boot/blob/eb7b6a776dd8b01849a0ea35d097b81f7eeec23e/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/diagnostics/analyzer/InvalidConfigurationPropertyNameFailureAnalyzer.java#L51-L52

Although you are right the docs I've written are inconsistent and use both. I'll switch to just one.

I don't think it makes sense to use flat string environment variables to pass them in, but instead need to pass them as structured data.

so yeah I thought this structured data is picked up from the application.properties? because how does it know what to set the values to ? i thought you need what you have in your docs as well?

It does get populated from the properties in the Environment, but these can come from any number of sources, not just the built-inapplication.properties.

See https://docs.spring.io/spring-boot/reference/features/external-config.html

The simplest way of doing it is probably through the command line arguments.

api/app/src/main/kotlin/packit/controllers/LoginController.kt

api/app/src/main/kotlin/packit/security/provider/TokenProvider.kt

plietar · 2024-09-25T13:01:30Z

The naming for this in other projects is kind of a mess:

GitHub calls this feature "OpenID Connect". I disagree with their naming, as there's nothing related about OIDC in the protocol. Both this scheme and OIDC use signed JWTs to prove identity, but OIDC is a much broader standard, and this scheme actually differs in some subtle points.
Buildkite also calls this OIDC, I suspect they'll have copied GitHub's naming
PyPI calls this "trusted publishers": https://docs.pypi.org/trusted-publishers/
Vault calls it jwt (or oidc when the actual OIDC bits are used): https://developer.hashicorp.com/vault/docs/auth/jwt
Kubernetes calls this "Bound Service Account Tokens": https://github.com/kubernetes/enhancements/blob/master/keps/sig-auth/1205-bound-service-account-tokens/README.md

I decided to avoid the OIDC name and just stick to JWT, or "External JWT" when it is ambiguous wrt Packit's own tokens. I'm happy to use a more catchy name if we want

plietar · 2024-09-25T13:02:18Z

This is an example of it being used in practice: https://github.com/plietar/orderly-ci-test/blob/f1987986d5fceeaf7b4cceb9aacf2708ae776f0a/run.R

absternator

Nice work!!!! looks great and i learn lots just reading it!! just left some comments about code structure

absternator · 2024-09-26T07:34:59Z

api/app/src/main/kotlin/packit/config/JwtLoginConfig.kt

+  val tokenDuration: Duration? = null,
+)
+
+@ConfigurationProperties(prefix = "auth.external-jwt")


do we need to add these into the applcicaton.properties? like the ones described in auth.md docs as well?

api/app/src/main/kotlin/packit/service/JwtLoginService.kt

api/app/src/main/kotlin/packit/controllers/LoginController.kt

absternator · 2024-09-26T07:52:58Z

api/app/src/main/kotlin/packit/config/JwtLoginConfig.kt

+  val tokenDuration: Duration? = null,
+)
+
+@ConfigurationProperties(prefix = "auth.external-jwt")


Suggested change

@ConfigurationProperties(prefix = "auth.external-jwt")

@ConfigurationProperties(prefix = "auth.externalJwt")

as per docs? or does it hyphen the camelcase?

absternator · 2024-09-26T08:25:17Z

api/app/src/main/kotlin/packit/config/JwtLoginConfig.kt

+@ConfigurationProperties(prefix = "auth.external-jwt")
+data class JwtLoginConfig(
+  val audience: String?,
+  val policy: List<JwtPolicy> = listOf()


policies instead of policy?

absternator · 2024-09-26T08:30:02Z

api/app/src/main/kotlin/packit/service/JwtLoginService.kt

+
+  val audience: String? = jwtLoginConfig.audience
+
+  init {


all the init should probs be in JwtLoginConfig and then it has the policies it can pass.. and thus we can keep the service clean

I'm not sure I agree. The JwtLoginConfig is a plain data class that just represents the properties. It seems odd that it would be initializing some business logic by creating validators and JWT decoders.

On the other hand I don't know if this type of init {} blocks is common or the right way to do this.

yeah that is fair but it seems like it does derive directly from config and doesn't need anything else right? and yeah the init in the service seems a bit odd.. maybe could create a bean with this logic and then directly inject into jwtlogin service?

something like this maybe?

@Configuration class JwtConfig { @Bean fun tokenPolicies(jwtLoginConfig: JwtLoginConfig, restOperations: RestOperations): List<TokenPolicy> { // init logic } }

then can pass into service straight up

class JwtLoginService( val policies: List<TokenPolicy>, val jwtIssuer: JwtIssuer, val userService: UserService, )

but if ya disagree feel free to keep your way 😄

I think I'll keep it this way. The contents of the TokenPolicy is so tightly coupled with the implementation of the service it doesn't really make sense to move it elsewhere. The only way the decoupling makes sense IMO is if we make the TokenPolicy an interface and move all the logic into it, at which point the service is nothing much more than a for-loop. Seems like an unnecessary abstraction, especially given there's only one kind of TokenPolicy.

I've changed it from a lateinit var to a val though, since I realised that works just as well.

absternator · 2024-09-26T08:39:11Z

api/app/src/main/kotlin/packit/security/provider/TokenProvider.kt

@@ -23,23 +39,59 @@ class TokenProvider(val config: AppConfig) : JwtIssuer
        const val TOKEN_AUDIENCE = "packit"
    }

-    override fun issue(userPrincipal: UserPrincipal): String
-    {
+    inner class Builder(val userPrincipal: UserPrincipal) : JwtBuilder {


the inner class stuff makes a bit messy... could move inner class outside and just pass along the config when calling instantiating class

absternator · 2024-09-26T08:42:02Z

api/app/src/main/kotlin/packit/security/provider/TokenProvider.kt

-    override fun issue(userPrincipal: UserPrincipal): String
-    {
+    inner class Builder(val userPrincipal: UserPrincipal) : JwtBuilder {
+        var duration: Duration = Duration.of(config.authExpiryDays, ChronoUnit.DAYS)


should probs be private

absternator · 2024-09-26T08:44:33Z

api/app/src/main/kotlin/packit/service/JwtLoginService.kt

+  }
+
+  private fun issueToken(verifiedToken: Jwt, policy: JwtPolicy): String {
+      val user = userService.getServiceUser()


also this all seems coupled to a service account... thus would a better name for all files and called be ServiceLogin...

Yeah makes sense

absternator · 2024-09-26T08:46:55Z

api/app/src/main/kotlin/packit/controllers/LoginController.kt

@@ -50,6 +52,31 @@ class LoginController(
        return ResponseEntity.ok(token)
    }

+    @PostMapping("/login/jwt")


as per other comment about naming.. maybe should be /login/service

This change allows automated environments, such as Github Actions or Buildkite to authenticate to the Packit API without needing to be manually provisioned with a secret. The client's environment creates and signs a JWT containing claims describing the job. For example, on Github Actions, GitHub provides a token which includes the organisation name, repository name, type of event, branch name, etc. Packit needs to be configured to trust the 3rd party JWT issuer, with policies specifying what claims are needed. The client sends the JWT it received from its environment to the Packit `/auth/login/jwt` endpoint, and in exchange receives a Packit JWT which may be used to access the rest of the API.

codecov · 2024-09-26T13:26:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.22%. Comparing base (223ba28) to head (8f2cb21).
Report is 11 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #131   +/-   ##
=======================================
  Coverage   97.22%   97.22%           
=======================================
  Files         131      131           
  Lines        1225     1225           
  Branches      339      339           
=======================================
  Hits         1191     1191           
  Misses         33       33           
  Partials        1        1

Flag	Coverage Δ
	`97.22% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

absternator

Nice work looks good to me!!! just a couple small comments but feel free to merge after response

api/app/src/main/kotlin/packit/controllers/LoginController.kt

Co-authored-by: Anmol Thapar <[email protected]>

plietar mentioned this pull request Sep 20, 2024

Add a service account. #130

Merged

plietar force-pushed the service-user branch from 726b921 to ca88e5c Compare September 23, 2024 14:30

plietar force-pushed the mrc-5763-oidc branch from 7ebb0d2 to 5e3187e Compare September 23, 2024 14:31

plietar force-pushed the service-user branch 6 times, most recently from 37f8836 to 7381cec Compare September 23, 2024 15:12

plietar force-pushed the mrc-5763-oidc branch 7 times, most recently from e5d4e31 to 3703e6e Compare September 24, 2024 16:33

plietar changed the title ~~OIDC~~ Add secret-less authentication for CI jobs. Sep 24, 2024

plietar marked this pull request as ready for review September 24, 2024 16:43

plietar force-pushed the mrc-5763-oidc branch 2 times, most recently from b587ff3 to ca82b18 Compare September 25, 2024 12:40

plietar requested a review from absternator September 25, 2024 12:40

plietar commented Sep 25, 2024

View reviewed changes

absternator reviewed Sep 26, 2024

View reviewed changes

plietar added 2 commits September 26, 2024 12:01

Code review

d8ce99d

plietar force-pushed the mrc-5763-oidc branch from 1430ec1 to 20c80b5 Compare September 26, 2024 12:17

plietar changed the base branch from service-user to main September 26, 2024 13:21

plietar requested a review from absternator September 26, 2024 13:21

Rename Jwt to Service.

d350824

plietar force-pushed the mrc-5763-oidc branch from 20c80b5 to d350824 Compare September 26, 2024 13:22

plietar added 2 commits September 26, 2024 14:38

fix tests

c1c4b64

fix tests again

b017d8d

absternator approved these changes Sep 27, 2024

View reviewed changes

api/app/src/main/kotlin/packit/controllers/LoginController.kt Outdated Show resolved Hide resolved

plietar and others added 2 commits September 27, 2024 09:54

Update api/app/src/main/kotlin/packit/controllers/LoginController.kt

b932e87

Co-authored-by: Anmol Thapar <[email protected]>

Make policies a val

8f2cb21

plietar merged commit 6ad0ede into main Sep 27, 2024
6 checks passed

plietar deleted the mrc-5763-oidc branch September 27, 2024 11:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add secret-less authentication for CI jobs. #131

Add secret-less authentication for CI jobs. #131

plietar commented Sep 20, 2024 •

edited

Loading

plietar Sep 24, 2024

absternator Sep 26, 2024

absternator Sep 26, 2024

plietar Sep 26, 2024

plietar Sep 26, 2024

absternator Sep 27, 2024

plietar Sep 27, 2024

plietar commented Sep 25, 2024

plietar commented Sep 25, 2024

absternator left a comment

absternator Sep 26, 2024

absternator Sep 26, 2024

absternator Sep 26, 2024

plietar Sep 26, 2024

absternator Sep 26, 2024

plietar Sep 26, 2024 •

edited

Loading

absternator Sep 27, 2024

plietar Sep 27, 2024

absternator Sep 26, 2024

plietar Sep 26, 2024

absternator Sep 26, 2024

plietar Sep 26, 2024

absternator Sep 26, 2024

plietar Sep 26, 2024

absternator Sep 26, 2024

codecov bot commented Sep 26, 2024 •

edited

Loading

absternator left a comment

	@ConfigurationProperties(prefix = "auth.external-jwt")
	@ConfigurationProperties(prefix = "auth.externalJwt")

Add secret-less authentication for CI jobs. #131

Add secret-less authentication for CI jobs. #131

Conversation

plietar commented Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plietar commented Sep 25, 2024

plietar commented Sep 25, 2024

absternator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plietar Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 26, 2024 • edited Loading

Codecov Report

absternator left a comment

Choose a reason for hiding this comment

plietar commented Sep 20, 2024 •

edited

Loading

plietar Sep 26, 2024 •

edited

Loading

codecov bot commented Sep 26, 2024 •

edited

Loading