Authentication flow

[mirkoDeer]

No description provided.

[cmdjulian]

lets make this a data class as well

[Copilot]

Pull request overview

This pull request implements a comprehensive authentication flow for container registries, replacing the previous authentication mechanism with a more robust session-based approach that supports both Bearer and Basic authentication schemes.

Changes:

• Implemented custom authentication providers (RegistryAuthProvider, RegistryBearerAuthProvider, RegistryBasicAuthProvider) with token caching and request coalescing
• Introduced authentication session management using coroutine context to share auth tokens across related requests
• Refactored upload and download flows to wrap operations in auth sessions and initialize authentication upfront
• Updated API layer to attach session IDs to all requests and simplified the execute method to handle transformations inline

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
kirc-suspending/src/main/kotlin/de/cmdjulian/kirc/impl/auth/RegistryAuthProvider.kt	New auth provider base class and implementations for Bearer and Basic authentication with caching
kirc-suspending/src/main/kotlin/de/cmdjulian/kirc/impl/auth/AuthSession.kt	Coroutine context-based session management for sharing auth tokens
kirc-suspending/src/main/kotlin/de/cmdjulian/kirc/impl/auth/AuthAttributes.kt	Request attributes for authentication flow control
kirc-suspending/src/main/kotlin/de/cmdjulian/kirc/impl/auth/BearerToken.kt	Renamed and updated token data class with expiry calculation
kirc-suspending/src/main/kotlin/de/cmdjulian/kirc/impl/ContainerRegistryApiImpl.kt	Refactored to attach session IDs and inline response transformations
kirc-suspending/src/main/kotlin/de/cmdjulian/kirc/impl/ContainerRegistryApi.kt	Added authChallenge method for initializing authentication
kirc-suspending/src/main/kotlin/de/cmdjulian/kirc/impl/SuspendingContainerImageRegistryClientImpl.kt	Added initializeAuth method and wrapped operations in auth sessions
kirc-suspending/src/main/kotlin/de/cmdjulian/kirc/impl/delegate/ImageUploader.kt	Wrapped upload flow in auth session with upfront authentication initialization
kirc-suspending/src/main/kotlin/de/cmdjulian/kirc/impl/delegate/ImageDownloader.kt	Wrapped download flow in auth session with upfront authentication initialization
kirc-suspending/src/main/kotlin/de/cmdjulian/kirc/client/SuspendingContainerImageClientFactory.kt	Updated to use new auth providers and inlined HTTPS configuration
kirc-suspending/src/main/kotlin/de/cmdjulian/kirc/client/SuspendingContainerImageRegistryClient.kt	Added initializeAuth and updated uploadBlobStream signature
kirc-suspending/src/main/kotlin/de/cmdjulian/kirc/utils/KtorInitUtils.kt	Removed - functionality moved to auth providers
kirc-suspending/src/main/kotlin/de/cmdjulian/kirc/utils/InsecureSSLComponents.kt	Removed - unused code
kirc-suspending/src/main/kotlin/de/cmdjulian/kirc/utils/ResponseTransformation.kt	Removed unused mapSuspending function
kirc-suspending/src/test/kotlin/de/cmdjulian/kirc/RegistryBearerAuthTest.kt	Updated tests for new upload modes and removed @disabled annotation
kirc-suspending/src/test/kotlin/de/cmdjulian/kirc/RegistryBasicAuthTest.kt	Added test for raw upload mode
kirc-suspending/src/test/kotlin/de/cmdjulian/kirc/AuthSessionTest.kt	New test file for auth session behavior
kirc-core/src/main/kotlin/de/cmdjulian/kirc/client/RegistryCredentials.kt	Changed from class to data class for proper equality semantics
docs/upload-graph.md	New documentation with sequence diagram for upload process
docs/download-graph.md	New documentation with sequence diagram for download process
kirc-suspending/build.gradle.kts	Added Caffeine cache and coroutines-test dependencies, removed ktor-client-logging

Comments suppressed due to low confidence (2)

kirc-suspending/src/main/kotlin/de/cmdjulian/kirc/impl/auth/BearerToken.kt:12

• The BearerToken class is declared as public (no visibility modifier) while it was previously internal as TokenResponse. Since this class is in the impl.auth package and appears to be an internal implementation detail, it should likely be marked as internal to prevent exposing implementation details to consumers of the library.
  kirc-suspending/src/main/kotlin/de/cmdjulian/kirc/impl/auth/BearerToken.kt:19
• The expiresAt method doesn't handle the case where expiresIn is null. When expiresIn is null but issuedAt is not null, the function returns issuedAt.plusSeconds(0), which means the token is immediately expired. This could lead to unexpected behavior. Consider either returning null when expiresIn is null, or using a sensible default expiry duration.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The inflightRequests ConcurrentHashMap uses computeIfAbsent with an async block. However, computeIfAbsent itself is not atomic when the mapping function performs asynchronous operations. If two coroutines call computeIfAbsent simultaneously with the same key, both could enter the mapping function before either completes, potentially creating duplicate token requests. While the Deferred.await() will ensure they both get the same result, this could still result in multiple token requests for the same session. Consider using a proper coroutine-based synchronization mechanism like Mutex with a check-lock-check pattern instead of relying on ConcurrentHashMap for this use case.

[cmdjulian]

Something like this might work:

package de.cmdjulian.kirc.impl.auth

import com.fasterxml.jackson.module.kotlin.readValue
import com.github.benmanes.caffeine.cache.Cache
import com.github.benmanes.caffeine.cache.Caffeine
import com.github.benmanes.caffeine.cache.Expiry
import de.cmdjulian.kirc.client.RegistryCredentials
import de.cmdjulian.kirc.impl.KircApiError
import de.cmdjulian.kirc.impl.serialization.JsonMapper
import io.ktor.client.HttpClient
import io.ktor.client.engine.cio.CIO
import io.ktor.client.plugins.auth.AuthProvider
import io.ktor.client.request.HttpRequestBuilder
import io.ktor.client.request.basicAuth
import io.ktor.client.request.bearerAuth
import io.ktor.client.request.get
import io.ktor.client.request.parameter
import io.ktor.client.statement.HttpResponse
import io.ktor.client.statement.bodyAsText
import io.ktor.client.statement.request
import io.ktor.http.HttpHeaders
import io.ktor.http.auth.AuthScheme
import io.ktor.http.auth.HttpAuthHeader
import io.ktor.http.auth.parseAuthorizationHeader
import io.ktor.util.Attributes
import kotlinx.coroutines.CompletableDeferred
import kotlinx.coroutines.Deferred
import kotlinx.coroutines.async
import kotlinx.coroutines.coroutineScope
import kotlinx.coroutines.sync.Mutex
import kotlinx.coroutines.sync.withLock
import java.util.*
import java.util.concurrent.ConcurrentHashMap
import kotlin.time.Duration.Companion.minutes
import kotlin.time.Duration.Companion.seconds

internal sealed class RegistryAuthProvider<T : Any>(private val credentials: RegistryCredentials?) : AuthProvider {

    protected companion object {
        val authHttpClient by lazy { HttpClient(CIO) }
    }

    @Deprecated("Please use sendWithoutRequest function instead")
    override val sendWithoutRequest get() = false

    // Mutex to guard the critical section of checking/creating inflight requests
    private val requestMutex = Mutex()
    
    // Kept as ConcurrentHashMap for thread-safe reads in non-suspend sendWithoutRequest
    protected val inflightRequests = ConcurrentHashMap<UUID, Deferred<T>>()
    
    protected val cache: Cache<UUID, T> = Caffeine.newBuilder()
        .expireAfter(
            object : Expiry<UUID, T> {
                override fun expireAfterCreate(key: UUID, value: T, currentTime: Long): Long = expireAfterCreate(value)
                override fun expireAfterUpdate(key: UUID, value: T, currentTime: Long, currentDuration: Long): Long = expireAfterCreate(value)
                override fun expireAfterRead(key: UUID, value: T, currentTime: Long, currentDuration: Long): Long = currentDuration
            },
        ).build()

    abstract fun expireAfterCreate(value: T): Long

    abstract override fun isApplicable(auth: HttpAuthHeader): Boolean

    abstract override suspend fun addRequestHeaders(request: HttpRequestBuilder, authHeader: HttpAuthHeader?)

    override fun sendWithoutRequest(request: HttpRequestBuilder): Boolean {
        // 1. Determine Identity
        val id = request.attributes.getSessionId() ?: return false

        // 2. Fast Path: Check Caffeine cache immediately
        cache.getIfPresent(id)?.let { return true }
        
        // This read is safe because inflightRequests is a ConcurrentHashMap
        return inflightRequests.containsKey(id)
    }

    override suspend fun refreshToken(response: HttpResponse): Boolean {
        val authHeader = parseAuthHeader(response) ?: return false

        if (authHeader.authScheme != AuthScheme.Bearer) return false

        val tokenResolved = resolveToken(response.request.attributes, authHeader) != null
        val skipRefresh = response.request.attributes.getOrNull(AuthAttributes.SKIP_REFRESH) == true
        return tokenResolved && !skipRefresh
    }

    protected suspend fun resolveToken(attributes: Attributes, authHeader: HttpAuthHeader?): T? {
        if (credentials == null) return null
        val id = attributes.getSessionId() ?: return null

        // 1. Fast Path: Check Caffeine cache immediately
        cache.getIfPresent(id)?.let { return it }

        if (authHeader !is HttpAuthHeader.Parameterized) return null
        val realm = authHeader.parameter("realm") ?: return null
        val scope = buildScope(attributes, authHeader)
        val service = authHeader.parameter("service")

        // 4. Slow Path: Request Coalescing (fetch token)
        return coroutineScope {
            // Use Mutex to ensure atomicity when checking/starting jobs
            val job = requestMutex.withLock {
                // Double-check: Did a job start while we were waiting for the lock?
                inflightRequests[id]?.let { return@withLock it }

                // Triple-check: Did a job finish and populate cache while we were waiting?
                cache.getIfPresent(id)?.let { return@withLock CompletableDeferred(it) }

                // Start new job
                val newJob = async {
                    try {
                        val token = requestToken(realm, scope, service, credentials)
                        cache.put(id, token)
                        token
                    } finally {
                        // Re-acquire lock to safely remove from map
                        requestMutex.withLock {
                            inflightRequests.remove(id)
                        }
                    }
                }
                inflightRequests[id] = newJob
                newJob
            }
            job.await()
        }
    }

    // ... (rest of the class remains the same: requestToken, parseAuthHeader, buildScope, getSessionId)
    
    protected abstract suspend fun requestToken(
        realm: String,
        scope: String?,
        service: String?,
        credentials: RegistryCredentials,
    ): T

    private fun parseAuthHeader(response: HttpResponse): HttpAuthHeader.Parameterized? {
        val headerValue = response.headers[HttpHeaders.WWWAuthenticate] ?: return null
        return try {
            parseAuthorizationHeader(headerValue) as? HttpAuthHeader.Parameterized
        } catch (_: Exception) {
            null
        }
    }

    private fun buildScope(attributes: Attributes, authHeader: HttpAuthHeader.Parameterized): String? {
        val repo = attributes.getOrNull(AuthAttributes.SCOPE_REPO)
        val type = attributes.getOrNull(AuthAttributes.SCOPE_TYPE)
        return if (repo != null && type != null) {
            "repository:$repo:$type"
        } else {
            authHeader.parameter("scope")
        }
    }

    private fun Attributes.getSessionId(): UUID? = getOrNull(AuthAttributes.SESSION_ID)?.let(UUID::fromString)
}

[Copilot]

The execute function documentation on line 71 contains redundant text that appears to be left over from editing. The phrase "If not, a [KircApiError.Unknown] is created with the original exception" is redundant and confusing since line 68 already states that if an exception is thrown, it is caught as KircApiError.Unknown. This redundant sentence should be removed.

Suggested change

	*
	* If not, a [KircApiError.Unknown] is created with the original exception

[Copilot]

The authHttpClient is created lazily as a shared singleton across all RegistryAuthProvider instances. This client is used for token retrieval but is never closed. Since HttpClient holds resources like connection pools and threads, this could lead to resource leaks if multiple provider instances are created and destroyed. Consider either closing the client when the provider is no longer needed or documenting that this is intentionally a long-lived singleton.

Suggested change

	/**
	* Shared HTTP client used for token retrieval across all [RegistryAuthProvider] instances.
	*
	* This client is intentionally a lazily initialized, process-wide singleton and is expected
	* to live for the lifetime of the application. It is therefore not closed here; shutting
	* it down, if required, must be handled at the application level.
	*/

[Copilot]

The test function name "upload - raw" is inconsistent with the naming pattern used in other test functions. The other upload tests use "upload stream" and "upload chunked" patterns. For consistency, this test should be named something like "upload - stream without assertions" or have a more descriptive name that explains what differentiates it from the other upload tests.

Suggested change

	fun `upload - raw`() {
	fun `upload stream - without assertions`() {

[Copilot]

The comment on line 61 contains a grammatical error. It should be "401 is expected if it is the first request in scope or if no credentials are provided" instead of "401 is expected if it is first request in scope no credentials are provided". The current wording is unclear and missing proper punctuation.

Suggested change

	// 401 is expected if it is first request in scope no credentials are provided
	// 401 is expected if it is the first request in scope or if no credentials are provided

[Copilot]

The refreshToken method in the base class hardcodes a check for AuthScheme.Bearer on line 99, which means RegistryBasicAuthProvider will always return false from this method. While Basic authentication doesn't typically require token refresh (credentials are sent with each request), this implicit behavior through inheritance is not obvious. Consider either overriding refreshToken in RegistryBasicAuthProvider to explicitly return false with a comment explaining why, or checking against isApplicable(authHeader) instead of hardcoding the Bearer scheme check.

[mirkoDeer]

good catch

[Copilot]

The documentation incorrectly specifies the scope as "pull,push" for a download operation. For download/pull operations, the scope should only be "pull", not "pull,push". The "pull,push" scope should only be used for upload operations.

Suggested change

	Client ->> Auth: GET /<realm>?scope=repository:<repo>:pull,push&service=<service>
	Client ->> Auth: GET /<realm>?scope=repository:<repo>:pull&service=<service>

[Copilot]

The currentSession function returns a new random UUID when no AuthSession is present in the coroutine context. This could lead to unexpected behavior where requests outside of withAuthSession blocks get different UUIDs on each call, preventing proper auth caching. Consider either requiring AuthSession to be present (throwing an exception if not found) or documenting this fallback behavior more explicitly in the function's KDoc.

Suggested change

	// Helper to retrieve the implicit ID or generate a new standalone one
	/**
	* Returns the current auth session ID from the coroutine context, if present.
	*
	* If an [AuthSession] is available in the current coroutine context, its [AuthSession.id]
	* is returned. If no [AuthSession] is present, a new random [UUID] is generated and
	* returned instead.
	*
	* Note:
	* - The fallback UUID is **not** stored in the coroutine context.
	* - Consecutive calls to this function outside of a [withAuthSession] block will therefore
	* each produce a different UUID.
	* - Callers that rely on a stable session ID for authentication caching must ensure they
	* execute within a [withAuthSession] scope.
	*/

[Copilot]

The new authentication provider classes (RegistryAuthProvider, RegistryBearerAuthProvider, RegistryBasicAuthProvider) and BearerToken class lack direct unit test coverage. While integration tests exist for bearer and basic auth flows, there are no tests for edge cases such as token expiry calculation, request coalescing behavior, cache eviction, or error handling in the auth provider logic. Consider adding unit tests for these components.

[Copilot]

The test function name "upload - raw" is inconsistent with the naming pattern used in other test functions. The other upload tests use "upload - stream" and similar patterns. For consistency and clarity, this test should have a more descriptive name that explains its purpose or how it differs from the "upload - stream" test below it.

Suggested change

	fun `upload - raw`() {
	fun `upload - stream without existence check`() {

[Copilot]

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 7 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The UUID.fromString() call on line 185 can throw IllegalArgumentException if the session ID string is malformed. While this is unlikely in normal operation, consider catching this exception and returning null to make the function more robust against malformed attributes.

Suggested change

	private fun Attributes.getSessionId(): UUID? = getOrNull(AuthAttributes.SESSION_ID)?.let(UUID::fromString)
	private fun Attributes.getSessionId(): UUID? {
	val sessionId = getOrNull(AuthAttributes.SESSION_ID) ?: return null
	return try {
	UUID.fromString(sessionId)
	} catch (_: IllegalArgumentException) {
	null
	}
	}

[Copilot]

If requestToken throws an exception, it will be propagated to the awaiting job but the inflightRequests entry will still be removed in the finally block. However, if the exception occurs, the cache won't be populated, which is correct. But consider whether subsequent requests should retry immediately or if failed auth attempts should have a short cooldown period to avoid hammering the auth server on repeated failures.

Suggested change

	try {
	val token = requestToken(realm, scope, service, credentials)
	cache.put(id, token)
	token
	} finally {
	// Re-acquire lock to safely remove from map
	requestMutex.withLock {
	inflightRequests.remove(id)
	var success = false
	try {
	val token = requestToken(realm, scope, service, credentials)
	cache.put(id, token)
	success = true
	token
	} finally {
	if (success) {
	// Re-acquire lock to safely remove from map
	requestMutex.withLock {
	inflightRequests.remove(id)
	}

[Copilot]

In the toErrorResponse function, calling bodyAsText() after body() has already been called will fail because the response body can only be consumed once. If body() throws an exception, the body is already partially or fully consumed, so bodyAsText() in the error message may fail or return empty content. Consider reading the body as text first and then parsing it, or removing the bodyAsText() call from the error message.

Suggested change

	message = "Could not parse registry error response body (body=${bodyAsText()})",
	message = "Could not parse registry error response body",

[Copilot]

There's a potential race condition in sendWithoutRequest between checking the cache (line 93) and checking inflightRequests (line 94). If a request completes and removes itself from inflightRequests between these two checks, the function might return false even though the token is now in the cache. While this is a minor issue that would just trigger an unnecessary auth flow, consider checking the cache again after checking inflightRequests, or checking both atomically.

Suggested change

	return inflightRequests.contains(id)
	// 3. If not in cache, check if there is an inflight request for this id
	if (inflightRequests.containsKey(id)) return true
	// 4. Race window handling:
	// Between the inflight check and here, the inflight request might have completed
	// and put the token into the cache. Re-check the cache to avoid an unnecessary
	// auth flow in that case.
	return cache.getIfPresent(id) != null

[Copilot]

The documentation for currentSession() states that it returns "a new random UUID" as fallback when no AuthSession is present, but then warns that "Consecutive calls to this function outside a withAuthSession block will therefore each produce a different UUID." This behavior could lead to unexpected issues where each request gets a different session ID, preventing token reuse. Consider clarifying if this is intentional behavior or if callers should always use withAuthSession.

Suggested change

	* Note:
	* - The fallback UUID is **not** stored in the coroutine context.
	* - Consecutive calls to this function outside a [withAuthSession] block will therefore each produce a different UUID.
	* - Callers that rely on a stable session ID for authentication caching must ensure they execute within a [withAuthSession] scope.
	* Important:
	* - The fallback UUID is **not** stored in the coroutine context and therefore does **not**
	* represent a real session. It is suitable only for non-critical use cases such as
	* logging or correlation where a stable session ID is not required.
	* - Consecutive calls to this function outside a [withAuthSession] block will each produce
	* a different UUID, and no token or authentication state will be reused between calls.
	* - Any code that relies on a stable, reusable session ID (for example, to reuse bearer
	* authentication tokens or cache authentication state) **must** execute within a
	* [withAuthSession] scope, and should obtain the ID via [currentSession] only from
	* within that scope.

[Copilot]

The documentation link appears to use an unusual domain "docker-docs.uclv.cu". Consider verifying this is the correct official Docker Registry documentation URL or update to use the official Docker documentation domain.

Suggested change

	* According to format definition [see](https://docker-docs.uclv.cu/registry/spec/api/#errors)
	* According to format definition [see](https://docs.docker.com/registry/spec/api/#errors)

[Copilot]

The authHttpClient is lazily initialized but never closed. This could lead to resource leaks since HttpClient holds resources like connection pools. Consider implementing a cleanup mechanism or using a shared client instance that is properly managed at the lifecycle level.