Skip to content

Fixes #29113: Security improvements for OIDC config #974

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
clarktsiory wants to merge 1 commit into
base: branches/rudder/9.0
Choose a base branch
from
+125 −33
Open

Fixes #29113: Security improvements for OIDC config #974

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions auth-backends/src/main/scala/bootstrap/rudder/plugin/AuthBackendsConf.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -582,6 +582,7 @@ class AuthBackendsSpringConfiguration extends ApplicationContextAware {
new RudderOpaqueTokenAuthenticationProvider(
introspector,
new RudderOpaqueTokenAuthenticationConverter(RudderConfig.roApiAccountRepository, config),
RudderConfig.roApiAccountRepository,
config.cacheRequestDuration,
() => Instant.now()
)
Expand Down Expand Up @@ -1375,10 +1376,11 @@ object RudderOpaqueTokenAuthenticationProvider {

// this class is only here to allow to use our convert in place of default spring configuration
class RudderOpaqueTokenAuthenticationProvider(
introspector: OpaqueTokenIntrospector,
converter: OpaqueTokenAuthenticationConverter,
validationCache: Option[Duration],
now: () => Instant
introspector: OpaqueTokenIntrospector,
converter: OpaqueTokenAuthenticationConverter,
roApiAccountRepository: RoApiAccountRepository,
validationCache: Option[Duration],
now: () => Instant
) extends AuthenticationProvider {
private val opaqueTokenAuthenticationProvider = new OpaqueTokenAuthenticationProvider(introspector)
opaqueTokenAuthenticationProvider.setAuthenticationConverter(converter)
Expand Down Expand Up @@ -1433,7 +1435,18 @@ class RudderOpaqueTokenAuthenticationProvider(
case Left(ex) => throw ex
// we do have an existing authentication.
// Expiration will be checked in convert.
case Right(auth @ RudderOAuth2OpaqueToken(ta, _)) =>
case Right(auth @ RudderOAuth2OpaqueToken(ta, u)) =>
// Rudder status check on API account to prevent disabled users during cache lifetime
u.account match {
case RudderAccount.Api(account) =>
roApiAccountRepository.getById(account.id).map(_.map(_.isEnabled)).runNow match {
case Some(true) => () // ok, next check
case Some(false) => throw new InvalidBearerTokenException(s"Token is disabled by Rudder API account")
case None => throw new InvalidBearerTokenException(s"Token is no longer mapped to a Rudder API account")
}
case _ =>
throw new InvalidBearerTokenException(s"Token is not mapped to a Rudder API account")
}
ta.getCredentials match {
case x: OAuth2AccessToken =>
if (now().isAfter(x.getExpiresAt)) {
Expand Down
18 changes: 9 additions & 9 deletions auth-backends/src/main/scala/com/normation/plugins/authbackends/Oauth2Authentication.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ object RudderRegistrationPropertyCommon {
A_TENANTS_OVERRIDE -> "(default false) keep user configured tenants in rudder-user.xml or override them with the one provided in the token",
A_TENANTS_MAPPING -> s"(optional) provides a map of alias `IdP tenant name` -> `Rudder tenant name`, where each IdP tenant name is a sub-key of '${A_TENANTS_MAPPING}'",
A_TENANTS_REVERSE_MAPPING -> s"(optional) provides a map of alias `Rudder tenant name` -> `IdP tenant name`, where each IdP tenant name is a sub-key of '${A_TENANTS_MAPPING}', useful when the IdP tenant name contains '='",
A_ENFORCE_TENANTS_MAPPING -> "(default true) if true, restricts roles available by the IdP to the role defined in mapping entitlement. Else the map provides alias for Rudder internal role names.",
A_ENFORCE_TENANTS_MAPPING -> "(default true) if true, restricts tenants available by the IdP to the tenants defined in mapping entitlement. Else the map provides id for Rudder tenants.",
A_URI_AUTH -> "provider URL to contact for main authentication (see provider documentation)",
A_URI_TOKEN -> "provider URL to contact for token verification (see provider documentation)",
A_URI_USER_INFO -> "provider URL to contact to get user information (see provider documentation)",
Expand Down Expand Up @@ -394,7 +394,7 @@ object RudderRegistrationPropertyCommon {
rolesEnabled <- read(A_ROLES_ENABLED).catchAll(_ => "false".succeed)
rolesAttr <- read(A_ROLES_ATTRIBUTE).catchAll(_ => "".succeed)
rolesOverride <- read(A_ROLES_OVERRIDE).catchAll(_ => "false".succeed)
enforceRoleMapping <- read(A_ENFORCE_ROLES_MAPPING).catchAll(_ => "false".succeed)
enforceRoleMapping <- read(A_ENFORCE_ROLES_MAPPING).catchAll(_ => "true".succeed)
mapping <- readMap(A_ROLES_MAPPING)
reverseMapping <- readMap(A_ROLES_REVERSE_MAPPING)
} yield {
Expand All @@ -410,18 +410,18 @@ object RudderRegistrationPropertyCommon {

protected[authbackends] def readTenants()(implicit base: BasePath, config: Config): IOResult[ProvidedTenants] = {
for {
tenantsEnabled <- read(A_TENANTS_ENABLED).catchAll(_ => "false".succeed)
tenantsAttr <- read(A_TENANTS_ATTRIBUTE).catchAll(_ => "".succeed)
tenantsOverride <- read(A_TENANTS_OVERRIDE).catchAll(_ => "false".succeed)
enforceRoleMapping <- read(A_ENFORCE_TENANTS_MAPPING).catchAll(_ => "false".succeed)
mapping <- readMap(A_TENANTS_MAPPING)
reverseMapping <- readMap(A_TENANTS_REVERSE_MAPPING)
tenantsEnabled <- read(A_TENANTS_ENABLED).catchAll(_ => "false".succeed)
tenantsAttr <- read(A_TENANTS_ATTRIBUTE).catchAll(_ => "".succeed)
tenantsOverride <- read(A_TENANTS_OVERRIDE).catchAll(_ => "false".succeed)
enforceTenantMapping <- read(A_ENFORCE_TENANTS_MAPPING).catchAll(_ => "true".succeed)
mapping <- readMap(A_TENANTS_MAPPING)
reverseMapping <- readMap(A_TENANTS_REVERSE_MAPPING)
} yield {
ProvidedTenants(
toBool(tenantsEnabled),
tenantsAttr,
toBool(tenantsOverride),
toBool(enforceRoleMapping),
toBool(enforceTenantMapping),
mapping ++ reverseMapping.map { case (a, b) => (b, a) }
)
}
Expand Down
117 changes: 98 additions & 19 deletions auth-backends/src/test/scala/com/normation/plugins/authbackends/TestCache.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,14 @@ package com.normation.plugins.authbackends

import bootstrap.rudder.plugin.RudderOAuth2OpaqueToken
import bootstrap.rudder.plugin.RudderOpaqueTokenAuthenticationProvider
import com.normation.rudder.api.ApiAccount
import com.normation.rudder.api.ApiAccountId
import com.normation.rudder.api.ApiAccountKind
import com.normation.rudder.api.ApiAccountName
import com.normation.rudder.api.ApiAuthorization
import com.normation.eventlog.*
import com.normation.rudder.MockApiAccountService
import com.normation.rudder.api.*
import com.normation.rudder.facts.nodes.NodeSecurityContext
import com.normation.rudder.users.RudderAccount
import com.normation.rudder.users.RudderUserDetail
import com.normation.rudder.users.UserStatus
import com.normation.zio.UnsafeRun
import java.time.Instant
import java.util.concurrent.TimeUnit
import org.joda.time.DateTime
Expand Down Expand Up @@ -77,20 +76,22 @@ class TestCache extends Specification {
private val TOKEN_EXP2 = Instant.ofEpochSecond(10 * 60)
private val TOKEN_EXP3 = Instant.ofEpochSecond(20 * 60)

private def apiAccount(s: String) = {
ApiAccount(
ApiAccountId(s),
ApiAccountKind.PublicApi(ApiAuthorization.RW, None),
ApiAccountName(s),
None,
"",
isEnabled = true,
new DateTime(0),
new DateTime(0),
NodeSecurityContext.All
)
}

private def rudderUserDetails(s: String) = RudderUserDetail(
RudderAccount.Api(
ApiAccount(
ApiAccountId(s),
ApiAccountKind.PublicApi(ApiAuthorization.RW, None),
ApiAccountName(s),
None,
"",
isEnabled = true,
new DateTime(0),
new DateTime(0),
NodeSecurityContext.All
)
),
RudderAccount.Api(apiAccount(s)),
UserStatus.Active,
Set(),
ApiAuthorization.RW,
Expand Down Expand Up @@ -142,12 +143,20 @@ class TestCache extends Specification {
}
}

private val apiAccountRepository = {
val mockService = new MockApiAccountService(null)
val repository = mockService.repository
(repository.save(apiAccount(GOOD_TOKEN_EXP1), ModificationId("test"), EventActor("test")) *>
repository.save(apiAccount(GOOD_TOKEN_EXP2), ModificationId("test"), EventActor("test"))).runNow
repository
}

// private val providerCache = new RudderOpaqueTokenAuthenticationProvider(introspector, converter, Some(Duration(4, TimeUnit.MINUTES)))

"when we don't have a cache" >> {
val introspector = new TestOpaqueTokenIntrospector
val providerNoCache =
new RudderOpaqueTokenAuthenticationProvider(introspector, converter, None, () => Instant.ofEpochSecond(4 * 60))
new RudderOpaqueTokenAuthenticationProvider(introspector, converter, null, None, () => Instant.ofEpochSecond(4 * 60))

"two times a correct value leads to 2 requests" in {

Expand All @@ -170,6 +179,7 @@ class TestCache extends Specification {
new RudderOpaqueTokenAuthenticationProvider(
introspector,
converter,
apiAccountRepository,
Some(Duration(5, TimeUnit.MINUTES)),
() => Instant.ofEpochSecond(4 * 60)
)
Expand Down Expand Up @@ -197,6 +207,7 @@ class TestCache extends Specification {
new RudderOpaqueTokenAuthenticationProvider(
introspector,
converter,
apiAccountRepository,
Some(Duration(5, TimeUnit.MINUTES)),
() => Instant.ofEpochSecond(c.toLong * 60)
)
Expand All @@ -215,4 +226,72 @@ class TestCache extends Specification {
introspector.count.get === 1
}
}

"when we have a cache and account is disabled" >> {
val introspector = new TestOpaqueTokenIntrospector
val c = 2
val providerCache = {
new RudderOpaqueTokenAuthenticationProvider(
introspector,
converter,
apiAccountRepository,
Some(Duration(5, TimeUnit.MINUTES)),
() => Instant.ofEpochSecond(c.toLong * 60)
)
}

"first request ok" in {
providerCache.authenticate(GOOD_TOKEN_EXP1.toAuth)
introspector.count.get === 1
}

"second request ko when disabled" in {
apiAccountRepository
.save(apiAccount(GOOD_TOKEN_EXP1).copy(isEnabled = false), ModificationId("test"), EventActor("test"))
.runNow
val res = Try(providerCache.authenticate(GOOD_TOKEN_EXP1.toAuth))

res must beAnInstanceOf[scala.util.Failure[?]]
introspector.count.get === 1
}

"third request ok when re-enabled" in {
apiAccountRepository.save(apiAccount(GOOD_TOKEN_EXP1), ModificationId("test"), EventActor("test")).runNow
providerCache.authenticate(GOOD_TOKEN_EXP1.toAuth)
introspector.count.get === 1
}
}

"when we have a cache and account is removed" >> {
val introspector = new TestOpaqueTokenIntrospector
val c = 2
val providerCache = {
new RudderOpaqueTokenAuthenticationProvider(
introspector,
converter,
apiAccountRepository,
Some(Duration(5, TimeUnit.MINUTES)),
() => Instant.ofEpochSecond(c.toLong * 60)
)
}

"first request ok" in {
providerCache.authenticate(GOOD_TOKEN_EXP1.toAuth)
introspector.count.get === 1
}

"second request ko when removed" in {
apiAccountRepository.delete(ApiAccountId(GOOD_TOKEN_EXP1), ModificationId("test"), EventActor("test")).runNow
val res = Try(providerCache.authenticate(GOOD_TOKEN_EXP1.toAuth))

res must beAnInstanceOf[scala.util.Failure[?]]
introspector.count.get === 1
}

"third request ok when added back" in {
apiAccountRepository.save(apiAccount(GOOD_TOKEN_EXP1), ModificationId("test"), EventActor("test")).runNow
providerCache.authenticate(GOOD_TOKEN_EXP1.toAuth)
introspector.count.get === 1
}
}
}