feat(discovery): JWT flow #1059

andrewazores · 2022-08-25T15:05:47Z

This PR implements the server side of discovery plugin JWT flow. HTTP_API.md contains a description of the flow as it relates to the endpoints.

The general idea is this:

To register as a plugin, a client must provide a unique realm name, which will become the root of a subtree in the overall discovery tree. This requires an Authorization header and standard authentication/authorization check, like most other API endpoints. If the request succeeds then Cryostat responds with a random UUID registration ID and a JWT access token. As with the other pre-existing JWT tokens, this one is signed and symmetrically encrypted, so it is completely opaque to the client or anyone else - only Cryostat can decrypt it.
To publish discovery updates, the plugin must POST to an endpoint with its registration UUID in the path and supply the access token as a query parameter. The access token includes information about the UUID and realm, as well as an expiry time, Cryostat's location (IP/host), and the location (IP) of the plugin. These factors are all verified to ensure that the client making the request is really the client for whom the token was originally generated. This ensures that no other API client, even ones that can pass a general Authorization check, are allowed to publish discovery subtrees to the plugin's realm.
To deregister itself the plugin DELETEs again to an endpoint with the registration UUID in the path and the access token as a query parameter. The same token factor checks are performed.
Since tokens expire, but the expiry information is embedded within the token and is unreadable to the plugin, Cryostat allows plugins to re-register themselves. This is the same as the original POST registration flow with a realm and callback, but additionally with the previous token (which for this purpose, may be expired - but all other factors must match). In this case, rather than registering a new plugin, Cryostat simply generates a refreshed token and responds to the plugin in the same format. The client can now use this refreshed token for its update or deregistration requests.
Plugins may choose to use 401 responses on update requests to determine that their token has expired, re-register to obtain a fresh one, and then re-attempt the update. Alternatively, Cryostat periodically pings plugins by their supplied callback URLs. The ping period is half of the token expiry time, so it is guaranteed that a ping occurs before a token expires. The plugin may choose to pre-emptively re-register itself for a fresh token every time it receives such a ping so that its token is always fresh.

chore(jwt): rename things to prep for introducing another JWT flow
feat(discovery): implement JWT flow
refactoring
remove outdated document
move docs into new directory
document discovery plugin API endpoints

maxcao13

Everything looks good to me.

I'm just wondering if the reason we are using HS256 over something like RSA256 is because cryostat is both the verifier and the provider of the token so there's no need to?
Also, what was the COMMANDS.md file and why was it removed?

maxcao13

Actually, I seem to get some problems when running sh smoketest.sh postgres with the pr.

Sep 02, 2022 11:38:01 PM io.cryostat.core.log.Logger error
SEVERE: HTTP 500: Cannot invoke "String.trim()" because "s" is null
io.vertx.ext.web.handler.HttpException: Internal Server Error
Caused by: io.cryostat.net.web.http.api.v2.ApiException: Cannot invoke "String.trim()" because "s" is null
        at io.cryostat.net.web.http.api.v2.AbstractDiscoveryJwtConsumingHandler.handle(AbstractDiscoveryJwtConsumingHandler.java:114)
        at io.cryostat.net.web.http.api.v2.AbstractDiscoveryJwtConsumingHandler.handle(AbstractDiscoveryJwtConsumingHandler.java:77)
        at io.vertx.ext.web.impl.BlockingHandlerDecorator.lambda$handle$0(BlockingHandlerDecorator.java:48)
        at io.vertx.core.impl.ContextImpl.lambda$null$0(ContextImpl.java:159)
        at io.vertx.core.impl.AbstractContext.dispatch(AbstractContext.java:100)
        at io.vertx.core.impl.ContextImpl.lambda$executeBlocking$1(ContextImpl.java:157)
        at io.vertx.core.impl.TaskQueue.run(TaskQueue.java:76)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
        at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
        at java.base/java.lang.Thread.run(Thread.java:833)
Caused by: java.lang.NullPointerException: Cannot invoke "String.trim()" because "s" is null
        at com.nimbusds.jose.JOSEObject.split(JOSEObject.java:218)
        at com.nimbusds.jose.JWEObject.parse(JWEObject.java:502)
        at io.cryostat.net.security.jwt.DiscoveryJwtHelper.parseDiscoveryPluginJwt(DiscoveryJwtHelper.java:150)
        at io.cryostat.net.security.jwt.DiscoveryJwtHelper.parseDiscoveryPluginJwt(DiscoveryJwtHelper.java:139)
        at io.cryostat.net.web.http.api.v2.AbstractDiscoveryJwtConsumingHandler.validateJwt(AbstractDiscoveryJwtConsumingHandler.java:147)
        at io.cryostat.net.web.http.api.v2.AbstractDiscoveryJwtConsumingHandler.handle(AbstractDiscoveryJwtConsumingHandler.java:108)
        ... 10 more
Sep 02, 2022 11:38:01 PM io.cryostat.core.log.Logger info
INFO: (10.0.2.100:54004): POST /api/v2.2/discovery/c3c3c16b-060b-41db-8cf8-dbdafafe298d 500 3ms
Hibernate: 
    /* insert io.cryostat.discovery.PluginInfo
        */ insert 
        into
            PluginInfo
            (callback, realm, subtree, id) 
        values
            (?, ?, ?, ?)
Sep 02, 2022 11:38:03 PM io.cryostat.core.log.Logger info
INFO: (10.0.2.100:33052): POST /api/v2.2/discovery 201 4ms
Hibernate: 
    select
        plugininfo0_.id as id1_0_0_,
        plugininfo0_.callback as callback2_0_0_,
        plugininfo0_.realm as realm3_0_0_,
        plugininfo0_.subtree as subtree4_0_0_ 
    from
        PluginInfo plugininfo0_ 
    where
        plugininfo0_.id=?
Sep 02, 2022 11:38:03 PM io.cryostat.core.log.Logger error
SEVERE: HTTP 500: Cannot invoke "String.trim()" because "s" is null
io.vertx.ext.web.handler.HttpException: Internal Server Error
Caused by: io.cryostat.net.web.http.api.v2.ApiException: Cannot invoke "String.trim()" because "s" is null
        at io.cryostat.net.web.http.api.v2.AbstractDiscoveryJwtConsumingHandler.handle(AbstractDiscoveryJwtConsumingHandler.java:114)
        at io.cryostat.net.web.http.api.v2.AbstractDiscoveryJwtConsumingHandler.handle(AbstractDiscoveryJwtConsumingHandler.java:77)
        at io.vertx.ext.web.impl.BlockingHandlerDecorator.lambda$handle$0(BlockingHandlerDecorator.java:48)
        at io.vertx.core.impl.ContextImpl.lambda$null$0(ContextImpl.java:159)
        at io.vertx.core.impl.AbstractContext.dispatch(AbstractContext.java:100)
        at io.vertx.core.impl.ContextImpl.lambda$executeBlocking$1(ContextImpl.java:157)
        at io.vertx.core.impl.TaskQueue.run(TaskQueue.java:76)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
        at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
        at java.base/java.lang.Thread.run(Thread.java:833)
Caused by: java.lang.NullPointerException: Cannot invoke "String.trim()" because "s" is null
        at com.nimbusds.jose.JOSEObject.split(JOSEObject.java:218)
        at com.nimbusds.jose.JWEObject.parse(JWEObject.java:502)
        at io.cryostat.net.security.jwt.DiscoveryJwtHelper.parseDiscoveryPluginJwt(DiscoveryJwtHelper.java:150)
        at io.cryostat.net.security.jwt.DiscoveryJwtHelper.parseDiscoveryPluginJwt(DiscoveryJwtHelper.java:139)
        at io.cryostat.net.web.http.api.v2.AbstractDiscoveryJwtConsumingHandler.validateJwt(AbstractDiscoveryJwtConsumingHandler.java:147)
        at io.cryostat.net.web.http.api.v2.AbstractDiscoveryJwtConsumingHandler.handle(AbstractDiscoveryJwtConsumingHandler.java:108)
        ... 10 more
Sep 02, 2022 11:38:03 PM io.cryostat.core.log.Logger info
INFO: (10.0.2.100:33052): POST /api/v2.2/discovery/c73a2fb2-a742-4ab4-988f-2692a10a0940 500 2ms
Hibernate: 
    /* insert io.cryostat.discovery.PluginInfo
        */ insert 
        into
            PluginInfo
            (callback, realm, subtree, id) 
        values
            (?, ?, ?, ?)
Sep 02, 2022 11:38:05 PM io.cryostat.core.log.Logger info
INFO: (10.0.2.100:33056): POST /api/v2.2/discovery 201 5ms
Hibernate: 
    select
        plugininfo0_.id as id1_0_0_,
        plugininfo0_.callback as callback2_0_0_,
        plugininfo0_.realm as realm3_0_0_,
        plugininfo0_.subtree as subtree4_0_0_ 
    from
        PluginInfo plugininfo0_ 
    where
        plugininfo0_.id=?

This pattern of messages pops up every 2 seconds after logging into the web-client and a different POST id is shown every time.

INFO: (10.0.2.100:33052): POST /api/v2.2/discovery/c73a2fb2-a742-4ab4-988f-2692a10a0940 500 2ms

...

INFO: (10.0.2.100:33056): POST /api/v2.2/discovery/3e475042-0727-4ba0-8357-5175a62b7b88 500 2ms

When I run with h2file I get a different error every 2 seconds:

Hibernate: 
    /* insert io.cryostat.discovery.PluginInfo
        */ insert 
        into
            PluginInfo
            (callback, realm, subtree, id) 
        values
            (?, ?, ?, ?)
Sep 02, 2022 11:42:09 PM org.hibernate.engine.jdbc.spi.SqlExceptionHelper logExceptions
WARN: SQL Error: 23505, SQLState: 23505
Sep 02, 2022 11:42:09 PM org.hibernate.engine.jdbc.spi.SqlExceptionHelper logExceptions
ERROR: Unique index or primary key violation: "UK_LFC8YJ7NOT9MTD4XYCNE72QEC_INDEX_5 ON PUBLIC.PLUGININFO(CALLBACK) VALUES ('http://cryostat:10010/cryostat-discovery', 4)"; SQL statement:
/* insert io.cryostat.discovery.PluginInfo */ insert into PluginInfo (callback, realm, subtree, id) values (?, ?, ?, ?) [23505-197]
Sep 02, 2022 11:42:09 PM org.hibernate.engine.jdbc.batch.internal.AbstractBatchImpl release
INFO: HHH000010: On release of batch it still contained JDBC statements
Sep 02, 2022 11:42:09 PM io.cryostat.core.log.Logger error
SEVERE: Exception thrown
javax.persistence.RollbackException: Error while committing the transaction
        at org.hibernate.internal.ExceptionConverterImpl.convertCommitException(ExceptionConverterImpl.java:81)
        at org.hibernate.engine.transaction.internal.TransactionImpl.commit(TransactionImpl.java:104)
        at io.cryostat.storage.AbstractDao.save(AbstractDao.java:71)
        at io.cryostat.discovery.PluginInfoDao.save(PluginInfoDao.java:74)
        at io.cryostat.discovery.DiscoveryStorage.register(DiscoveryStorage.java:160)
        at io.cryostat.net.web.http.api.v2.DiscoveryRegistrationHandler.handle(DiscoveryRegistrationHandler.java:155)
        at io.cryostat.net.web.http.api.v2.AbstractV2RequestHandler.handle(AbstractV2RequestHandler.java:123)
        at io.cryostat.net.web.http.api.v2.AbstractV2RequestHandler.handle(AbstractV2RequestHandler.java:75)
        at io.vertx.ext.web.impl.BlockingHandlerDecorator.lambda$handle$0(BlockingHandlerDecorator.java:48)
        at io.vertx.core.impl.ContextImpl.lambda$null$0(ContextImpl.java:159)
        at io.vertx.core.impl.AbstractContext.dispatch(AbstractContext.java:100)
        at io.vertx.core.impl.ContextImpl.lambda$executeBlocking$1(ContextImpl.java:157)
        at io.vertx.core.impl.TaskQueue.run(TaskQueue.java:76)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
        at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
        at java.base/java.lang.Thread.run(Thread.java:833)
Caused by: javax.persistence.PersistenceException: org.hibernate.exception.ConstraintViolationException: could not execute statement
        at org.hibernate.internal.ExceptionConverterImpl.convert(ExceptionConverterImpl.java:154)
        at org.hibernate.internal.ExceptionConverterImpl.convert(ExceptionConverterImpl.java:181)
        at org.hibernate.internal.ExceptionConverterImpl.convertCommitException(ExceptionConverterImpl.java:65)
        ... 16 more
Caused by: org.hibernate.exception.ConstraintViolationException: could not execute statement
        at org.hibernate.exception.internal.SQLStateConversionDelegate.convert(SQLStateConversionDelegate.java:109)
        at org.hibernate.exception.internal.StandardSQLExceptionConverter.convert(StandardSQLExceptionConverter.java:37)
        at org.hibernate.engine.jdbc.spi.SqlExceptionHelper.convert(SqlExceptionHelper.java:113)
        at org.hibernate.engine.jdbc.spi.SqlExceptionHelper.convert(SqlExceptionHelper.java:99)
        at org.hibernate.engine.jdbc.internal.ResultSetReturnImpl.executeUpdate(ResultSetReturnImpl.java:200)
        at org.hibernate.engine.jdbc.batch.internal.NonBatchingBatch.addToBatch(NonBatchingBatch.java:46)
        at org.hibernate.persister.entity.AbstractEntityPersister.insert(AbstractEntityPersister.java:3375)
        at org.hibernate.persister.entity.AbstractEntityPersister.insert(AbstractEntityPersister.java:3908)
        at org.hibernate.action.internal.EntityInsertAction.execute(EntityInsertAction.java:107)
        at org.hibernate.engine.spi.ActionQueue.executeActions(ActionQueue.java:604)
        at org.hibernate.engine.spi.ActionQueue.lambda$executeActions$1(ActionQueue.java:478)
        at java.base/java.util.LinkedHashMap.forEach(LinkedHashMap.java:721)
        at org.hibernate.engine.spi.ActionQueue.executeActions(ActionQueue.java:475)
        at org.hibernate.event.internal.AbstractFlushingEventListener.performExecutions(AbstractFlushingEventListener.java:344)
        at org.hibernate.event.internal.DefaultFlushEventListener.onFlush(DefaultFlushEventListener.java:40)
        at org.hibernate.event.service.internal.EventListenerGroupImpl.fireEventOnEachListener(EventListenerGroupImpl.java:107)
        at org.hibernate.internal.SessionImpl.doFlush(SessionImpl.java:1407)
        at org.hibernate.internal.SessionImpl.managedFlush(SessionImpl.java:489)
        at org.hibernate.internal.SessionImpl.flushBeforeTransactionCompletion(SessionImpl.java:3290)
        at org.hibernate.internal.SessionImpl.beforeTransactionCompletion(SessionImpl.java:2425)
        at org.hibernate.engine.jdbc.internal.JdbcCoordinatorImpl.beforeTransactionCompletion(JdbcCoordinatorImpl.java:449)
        at org.hibernate.resource.transaction.backend.jdbc.internal.JdbcResourceLocalTransactionCoordinatorImpl.beforeCompletionCallback(JdbcResourceLocalTransactionCoordinatorImpl.java:183)
        at org.hibernate.resource.transaction.backend.jdbc.internal.JdbcResourceLocalTransactionCoordinatorImpl.access$300(JdbcResourceLocalTransactionCoordinatorImpl.java:40)
        at org.hibernate.resource.transaction.backend.jdbc.internal.JdbcResourceLocalTransactionCoordinatorImpl$TransactionDriverControlImpl.commit(JdbcResourceLocalTransactionCoordinatorImpl.java:281)
        at org.hibernate.engine.transaction.internal.TransactionImpl.commit(TransactionImpl.java:101)
        ... 15 more
Caused by: org.h2.jdbc.JdbcSQLException: Unique index or primary key violation: "UK_LFC8YJ7NOT9MTD4XYCNE72QEC_INDEX_5 ON PUBLIC.PLUGININFO(CALLBACK) VALUES ('http://cryostat:10010/cryostat-discovery', 4)"; SQL statement:
/* insert io.cryostat.discovery.PluginInfo */ insert into PluginInfo (callback, realm, subtree, id) values (?, ?, ?, ?) [23505-197]
        at org.h2.message.DbException.getJdbcSQLException(DbException.java:357)
        at org.h2.message.DbException.get(DbException.java:179)
        at org.h2.message.DbException.get(DbException.java:155)
        at org.h2.index.BaseIndex.getDuplicateKeyException(BaseIndex.java:101)
        at org.h2.mvstore.db.MVSecondaryIndex.requireUnique(MVSecondaryIndex.java:236)
        at org.h2.mvstore.db.MVSecondaryIndex.add(MVSecondaryIndex.java:202)
        at org.h2.mvstore.db.MVTable.addRow(MVTable.java:732)
        at org.h2.command.dml.Insert.insertRows(Insert.java:182)
        at org.h2.command.dml.Insert.update(Insert.java:134)
        at org.h2.command.CommandContainer.update(CommandContainer.java:102)
        at org.h2.command.Command.executeUpdate(Command.java:261)
        at org.h2.jdbc.JdbcPreparedStatement.executeUpdateInternal(JdbcPreparedStatement.java:199)
        at org.h2.jdbc.JdbcPreparedStatement.executeUpdate(JdbcPreparedStatement.java:153)
        at org.hibernate.engine.jdbc.internal.ResultSetReturnImpl.executeUpdate(ResultSetReturnImpl.java:197)
        ... 35 more

Sep 02, 2022 11:42:09 PM io.cryostat.core.log.Logger warn
WARNING: HTTP 400: Failed to register new provider for "quarkus-test-ac4c4c7c-dbd0-4c8e-ada4-764317fef7af" @ "http://cryostat:10010/cryostat-discovery": provider already present
io.vertx.ext.web.handler.HttpException: Bad Request
Caused by: io.cryostat.net.web.http.api.v2.ApiException: Failed to register new provider for "quarkus-test-ac4c4c7c-dbd0-4c8e-ada4-764317fef7af" @ "http://cryostat:10010/cryostat-discovery": provider already present
        at io.cryostat.net.web.http.api.v2.DiscoveryRegistrationHandler.handle(DiscoveryRegistrationHandler.java:157)
        at io.cryostat.net.web.http.api.v2.AbstractV2RequestHandler.handle(AbstractV2RequestHandler.java:123)
        at io.cryostat.net.web.http.api.v2.AbstractV2RequestHandler.handle(AbstractV2RequestHandler.java:75)
        at io.vertx.ext.web.impl.BlockingHandlerDecorator.lambda$handle$0(BlockingHandlerDecorator.java:48)
        at io.vertx.core.impl.ContextImpl.lambda$null$0(ContextImpl.java:159)
        at io.vertx.core.impl.AbstractContext.dispatch(AbstractContext.java:100)
        at io.vertx.core.impl.ContextImpl.lambda$executeBlocking$1(ContextImpl.java:157)
        at io.vertx.core.impl.TaskQueue.run(TaskQueue.java:76)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
        at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
        at java.base/java.lang.Thread.run(Thread.java:833)
Caused by: io.cryostat.discovery.RegistrationException: Failed to register new provider for "quarkus-test-ac4c4c7c-dbd0-4c8e-ada4-764317fef7af" @ "http://cryostat:10010/cryostat-discovery": provider already present
        at io.cryostat.discovery.DiscoveryStorage.register(DiscoveryStorage.java:165)
        at io.cryostat.net.web.http.api.v2.DiscoveryRegistrationHandler.handle(DiscoveryRegistrationHandler.java:155)
        ... 11 more
Caused by: javax.persistence.RollbackException: Error while committing the transaction
        at org.hibernate.internal.ExceptionConverterImpl.convertCommitException(ExceptionConverterImpl.java:81)
        at org.hibernate.engine.transaction.internal.TransactionImpl.commit(TransactionImpl.java:104)
        at io.cryostat.storage.AbstractDao.save(AbstractDao.java:71)
        at io.cryostat.discovery.PluginInfoDao.save(PluginInfoDao.java:74)
        at io.cryostat.discovery.DiscoveryStorage.register(DiscoveryStorage.java:160)
        ... 12 more
Caused by: javax.persistence.PersistenceException: org.hibernate.exception.ConstraintViolationException: could not execute statement
        at org.hibernate.internal.ExceptionConverterImpl.convert(ExceptionConverterImpl.java:154)
        at org.hibernate.internal.ExceptionConverterImpl.convert(ExceptionConverterImpl.java:181)
        at org.hibernate.internal.ExceptionConverterImpl.convertCommitException(ExceptionConverterImpl.java:65)
        ... 16 more
Caused by: org.hibernate.exception.ConstraintViolationException: could not execute statement
        at org.hibernate.exception.internal.SQLStateConversionDelegate.convert(SQLStateConversionDelegate.java:109)
        at org.hibernate.exception.internal.StandardSQLExceptionConverter.convert(StandardSQLExceptionConverter.java:37)
        at org.hibernate.engine.jdbc.spi.SqlExceptionHelper.convert(SqlExceptionHelper.java:113)
        at org.hibernate.engine.jdbc.spi.SqlExceptionHelper.convert(SqlExceptionHelper.java:99)
        at org.hibernate.engine.jdbc.internal.ResultSetReturnImpl.executeUpdate(ResultSetReturnImpl.java:200)
        at org.hibernate.engine.jdbc.batch.internal.NonBatchingBatch.addToBatch(NonBatchingBatch.java:46)
        at org.hibernate.persister.entity.AbstractEntityPersister.insert(AbstractEntityPersister.java:3375)
        at org.hibernate.persister.entity.AbstractEntityPersister.insert(AbstractEntityPersister.java:3908)
        at org.hibernate.action.internal.EntityInsertAction.execute(EntityInsertAction.java:107)
        at org.hibernate.engine.spi.ActionQueue.executeActions(ActionQueue.java:604)
        at org.hibernate.engine.spi.ActionQueue.lambda$executeActions$1(ActionQueue.java:478)
        at java.base/java.util.LinkedHashMap.forEach(LinkedHashMap.java:721)
        at org.hibernate.engine.spi.ActionQueue.executeActions(ActionQueue.java:475)
        at org.hibernate.event.internal.AbstractFlushingEventListener.performExecutions(AbstractFlushingEventListener.java:344)
        at org.hibernate.event.internal.DefaultFlushEventListener.onFlush(DefaultFlushEventListener.java:40)
        at org.hibernate.event.service.internal.EventListenerGroupImpl.fireEventOnEachListener(EventListenerGroupImpl.java:107)
        at org.hibernate.internal.SessionImpl.doFlush(SessionImpl.java:1407)
        at org.hibernate.internal.SessionImpl.managedFlush(SessionImpl.java:489)
        at org.hibernate.internal.SessionImpl.flushBeforeTransactionCompletion(SessionImpl.java:3290)
        at org.hibernate.internal.SessionImpl.beforeTransactionCompletion(SessionImpl.java:2425)
        at org.hibernate.engine.jdbc.internal.JdbcCoordinatorImpl.beforeTransactionCompletion(JdbcCoordinatorImpl.java:449)
        at org.hibernate.resource.transaction.backend.jdbc.internal.JdbcResourceLocalTransactionCoordinatorImpl.beforeCompletionCallback(JdbcResourceLocalTransactionCoordinatorImpl.java:183)
        at org.hibernate.resource.transaction.backend.jdbc.internal.JdbcResourceLocalTransactionCoordinatorImpl.access$300(JdbcResourceLocalTransactionCoordinatorImpl.java:40)
        at org.hibernate.resource.transaction.backend.jdbc.internal.JdbcResourceLocalTransactionCoordinatorImpl$TransactionDriverControlImpl.commit(JdbcResourceLocalTransactionCoordinatorImpl.java:281)
        at org.hibernate.engine.transaction.internal.TransactionImpl.commit(TransactionImpl.java:101)
        ... 15 more
Caused by: org.h2.jdbc.JdbcSQLException: Unique index or primary key violation: "UK_LFC8YJ7NOT9MTD4XYCNE72QEC_INDEX_5 ON PUBLIC.PLUGININFO(CALLBACK) VALUES ('http://cryostat:10010/cryostat-discovery', 4)"; SQL statement:
/* insert io.cryostat.discovery.PluginInfo */ insert into PluginInfo (callback, realm, subtree, id) values (?, ?, ?, ?) [23505-197]
        at org.h2.message.DbException.getJdbcSQLException(DbException.java:357)
        at org.h2.message.DbException.get(DbException.java:179)
        at org.h2.message.DbException.get(DbException.java:155)
        at org.h2.index.BaseIndex.getDuplicateKeyException(BaseIndex.java:101)
        at org.h2.mvstore.db.MVSecondaryIndex.requireUnique(MVSecondaryIndex.java:236)
        at org.h2.mvstore.db.MVSecondaryIndex.add(MVSecondaryIndex.java:202)
        at org.h2.mvstore.db.MVTable.addRow(MVTable.java:732)
        at org.h2.command.dml.Insert.insertRows(Insert.java:182)
        at org.h2.command.dml.Insert.update(Insert.java:134)
        at org.h2.command.CommandContainer.update(CommandContainer.java:102)
        at org.h2.command.Command.executeUpdate(Command.java:261)
        at org.h2.jdbc.JdbcPreparedStatement.executeUpdateInternal(JdbcPreparedStatement.java:199)
        at org.h2.jdbc.JdbcPreparedStatement.executeUpdate(JdbcPreparedStatement.java:153)
        at org.hibernate.engine.jdbc.internal.ResultSetReturnImpl.executeUpdate(ResultSetReturnImpl.java:197)
        ... 35 more
Sep 02, 2022 11:42:09 PM io.cryostat.core.log.Logger info
INFO: (10.0.2.100:49182): POST /api/v2.2/discovery 400 5ms

andrewazores · 2022-09-06T14:12:17Z

I'll try to reproduce those failures, thanks for detailing them.

I'm just wondering if the reason we are using HS256 over something like RSA256 is because cryostat is both the verifier and the provider of the token so there's no need to?
Also, what was the COMMANDS.md file and why was it removed?

Right, HS256 is used because it's symmetric - there is just one key and it is randomly generated each time a Cryostat instance is started. Since Cryostat is both the creator and consumer of the token and we don't need or want any other client that sees the token to be able to read it this works well.

COMMANDS.md was the precursor to HTTP_API.md. It described the old interactive command channel API interface. That has been gone for a long time but the document describing it was missed when everything else got deleted.

andrewazores · 2022-09-06T14:50:36Z

Ah, right. The 500 in there is an actual bug that I'll push a minor fix for in a moment. The other failures showing up in those logs are actually just because the quarkus-test sample application deployed in smoketest.sh doesn't (yet) implement the updated JWT flow, so it's failing to properly register itself, then retrying and failing again ad infinitum. I was testing this out with an updated build of the cryostat-agent hooked up to the -reports sidecar instead. I'll touch that up and add some instructions on how to test with that setup too. Once we have settled on the server-side API details then I will make any required adjustments to -agent as well as to quarkus-test.

Do you have any feedback or questions about the JWT additions at this point?

maxcao13 · 2022-09-06T16:56:43Z

Thanks for the clarification on my previous questions!

Do you have any feedback or questions about the JWT additions at this point?

I guess I'm wondering what JWT authentication provides in addition to the authentication cryostat already had beforehand (e.g. the AuthManager interface and its implementations).

Also, not really related to this PR but cryostat JWT in general, when does it make sense to include JWT auth vs. not to in an endpoint? For example, v2 RulesGetHandler does not use JWT but v2 ReportGetHandler does.

andrewazores · 2022-09-06T17:58:20Z

Also, not really related to this PR but cryostat JWT in general, when does it make sense to include JWT auth vs. not to in an endpoint? For example, v2 RulesGetHandler does not use JWT but v2 ReportGetHandler does.

The older set of v2 JWT-based handlers, like the ReportGetHandler that you mentioned, are just meant as an authentication/authorization alternative to their v1 equivalents. The original v1s all require the client to supply an Authorization header with the request (unless the NoopAuthManager is selected). For programmatic clients this is no problem, however for human users interacting through a web browser it becomes a problem because browsers want to use cookies for the Authorization header when performing file downloads and won't allow us to set arbitrary headers either (like X-JMX-Authorization), and we also don't implement cookie support for auth on the backend. Rather than adding cookies and session-based auth etc, I worked around this by implementing the v2 JWT handlers.

These work by allowing the web-client running in the browser to perform an API request with our normal Authorization header flow, where the client making the request also specifies what resource they want to download. Cryostat performs an auth check on the Authorization header and if this passes it generates a token to send back to the client. This token is encrypted and signed by Cryostat itself so that to the receiving client it's just an opaque string of characters with no discernible meaning, and any modifications to the text will invalidate the token (signature failure). The client can then make a v2 API request to download the resource it actually wants (ex. a report) and include the token as a query parameter in the request without having to set any request headers. Cryostat then receives that token, checks the signature and decrypts it - if these steps succeed then we know that the token was generated by the current Cryostat instance and was not subsequently modified. The token also contains information about who the requesting client is (the Authorization header they originally passed, which hooks back into the AuthManager implementations) so we can ensure that the client is authorized to access this resource, what the intended resource it was generated for was so that we can check that vs. the actual resource URL that the client is trying to use the token for, as well as contains information about JMX credentials if required, and the Cryostat instance's location, as well as the timestamp when the token was generated and how long it's valid for. If all of this checks out then the request passes and we send the requested resource back to the client.

All of this is to say that the original JWT handlers are there essentially just to make the web-client work better, so that we can use the standard browser file download functionality. Originally that functionality was hacked around by having the web-client make an Authorization-header request to the v1 API endpoints and download the response fully into browser memory as a Blob, then save that to file after the download completed. This meant the user never had the chance to cancel a request or cancel an in-progress download, and the web-client couldn't be closed without interrupting the download either.

Right now we still have this distinct split between using the Authorization header vs using JWT, but I hope that in a future major release version (3.0?) we can unify this and either adopt JWT for everything or have an implementation that allows either to be used interchangeably depending on the client's capabilities. The downside to using JWT for everything is that it forces the client to make a request for a JWT and then follow up with the actual request they wanted to make, so it's more resource intensive and is subject to double the network latency. For that reason, JWT is only really used for file GET/download requests that might be initiated by the web-client. Everything else uses the standard Authorization header style.

andrewazores · 2022-09-06T18:06:47Z

I guess I'm wondering what JWT authentication provides in addition to the authentication cryostat already had beforehand (e.g. the AuthManager interface and its implementations).

In the context of this PR and the use of JWT w.r.t. the Pluggable Discovery API, many of the same concepts as in my previous response are still valid. The plugins register themselves using a standard request with an Authorization header, at which point a JWT is generated and sent back. Subsequent requests for publishing discovery subtrees or for deregistering use the JWT rather than the Authorization header. The JWT is again signed and encrypted, and this time it contains information about the plugin's discovery subtree Realm. In practical terms this means that any client that can pass an Authorization check can register itself as a discovery plugin. But, once such a registration occurs for a given Realm name, then only a client bearing a JWT for that Realm is able to perform deregistrations or to publish subtrees for that Realm. It essentially allows Cryostat to set a sub scope of authorization for the subtree Realm to only one client at a time. The current implementation before this PR using only the Authorization header allows for any client to theoretically publish subtree updates to any Realm, or even to deregister other plugins and delete their Realms. The only mitigation against this prior to this PR is that the plugin registration uses a UUID instead of a simple incrementing numeric ID, so the IDs are very difficult to predict. But if you do guess a valid registration UUID and can pass an Authorization check then you can freely publish discovery updates to that plugin's Realm or deregister the plugin. The IDs are still UUIDs and the JWTs expire fairly quickly, so the only way a client can publish subtree updates or deregister is to both know the correct plugin UUID as well as have a current JWT, both of which aren't going to happen by guessing or by accident.

On discovery plugin registration, server generates a JWT and supplies it to the plugin. This JWT must be supplied by the plugin in place of Authorization header for discovery updates and deregistration. Server's existing periodic ping task to check plugin reachability should prompt plugin to attempt to re-register itself, but this time it has an existing valid token to supply. If the server gets a registration request that includes a refreshed token is generated and supplied back into the same registration request flow.

startup ping is a GET, later pruning pings are POST. plugins should simply respond with a 2xx status to a GET and sit idle, but after a POST they should respond with a 2xx and then call back to the Cryostat registration endpoint to refresh their access token

maxcao13 · 2022-09-07T16:36:29Z

Ahh, thanks so much, your write up was very insightful into the back-end auth code and its motivation! I understand a lot better now.

andrewazores · 2022-09-08T13:26:25Z

I've updated the PR with a few other minor fixes/changes.

When a plugin tries to register itself, Cryostat sends a GET to the provided callback URI. This is to ensure that the URI is actually something reachable, not just a valid format. Plugins are only expected to send a 2xx response here.
The periodic ping and the startup ping use a POST to the callback URIs. Plugins should send a 2xx response, and may also wish to perform a re-registration to refresh their JWT, though this is not strictly required.
The 500 response when no token is supplied is now a 401
quarkus-test-plugin in the smoketest script has been updated to a newer version that fully implements the registration/update/reregistration/deregistration behaviours.

maxcao13

Other than that, after going through everything the other day, everything looks good. Unless you still want to implement that feature of non-unique names of plugins that will be appended by some random id.

Good work!

smoketest.sh

andrewazores · 2022-09-12T18:37:53Z

Other than that, after going through everything the other day, everything looks good. Unless you still want to implement that feature of non-unique names of plugins that will be appended by some random id.

Good work!

I'll follow that up in a separate PR

andrewazores · 2022-09-12T18:40:51Z

Other than that, after going through everything the other day, everything looks good. Unless you still want to implement that feature of non-unique names of plugins that will be appended by some random id.
Good work!

I'll follow that up in a separate PR

#1061

andrewazores added the feat New feature or request label Aug 25, 2022

andrewazores mentioned this pull request Aug 25, 2022

feat(jwt): implement JWT flow cryostatio/cryostat-agent#9

Merged

andrewazores marked this pull request as ready for review August 25, 2022 21:29

andrewazores requested review from maxcao13 and tthvo August 25, 2022 21:30

maxcao13 previously approved these changes Sep 2, 2022

View reviewed changes

maxcao13 reviewed Sep 2, 2022

View reviewed changes

andrewazores dismissed maxcao13’s stale review via 2e5b044 September 6, 2022 14:52

andrewazores force-pushed the discovery-plugin-jwt branch from e090840 to 2e5b044 Compare September 6, 2022 14:52

andrewazores force-pushed the discovery-plugin-jwt branch from 2e5b044 to eadd109 Compare September 6, 2022 18:09

andrewazores added 13 commits September 6, 2022 14:45

chore(jwt): rename things to prep for introducing another JWT flow

3b9ba6d

refactoring

f5b3ca5

remove outdated document

2fd0a17

move docs into new directory

d9357dc

document discovery plugin API endpoints

9a35f9b

re-registration must match original realm and callback

41a99a2

tokens expire after 2*ping_period

4077439

fix unit tests

8b54947

minor fixes, updated itests

844d6e7

fix spotbugs warnings

07aa764

fixup! fix spotbugs warnings

6f18127

throw 401 is token is expected and not provided

6bcb816

andrewazores force-pushed the discovery-plugin-jwt branch from eadd109 to 6bcb816 Compare September 6, 2022 18:45

update plugin ping logic

8ab77f2

startup ping is a GET, later pruning pings are POST. plugins should simply respond with a 2xx status to a GET and sit idle, but after a POST they should respond with a 2xx and then call back to the Cryostat registration endpoint to refresh their access token

andrewazores added 4 commits September 6, 2022 23:02

use later quarkus-test that implements JWT flow

4897f5c

fixup! update plugin ping logic

0d4d108

fixup! use later quarkus-test that implements JWT flow

e02bebe

document POST/GET ping difference

6dd41f6

time-expired tokens may still deregister plugins

ffb7806

maxcao13 reviewed Sep 12, 2022

View reviewed changes

smoketest.sh Show resolved Hide resolved

document quarkus-test-plugin 10010 port

2a8d831

maxcao13 approved these changes Sep 12, 2022

View reviewed changes

andrewazores merged commit 58bc83d into cryostatio:main Sep 12, 2022

andrewazores deleted the discovery-plugin-jwt branch September 12, 2022 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(discovery): JWT flow #1059

feat(discovery): JWT flow #1059

andrewazores commented Aug 25, 2022 •

edited

maxcao13 left a comment

maxcao13 left a comment •

edited

andrewazores commented Sep 6, 2022

andrewazores commented Sep 6, 2022 •

edited

maxcao13 commented Sep 6, 2022 •

edited

andrewazores commented Sep 6, 2022

andrewazores commented Sep 6, 2022

maxcao13 commented Sep 7, 2022

andrewazores commented Sep 8, 2022

maxcao13 left a comment

andrewazores commented Sep 12, 2022

andrewazores commented Sep 12, 2022

feat(discovery): JWT flow #1059

feat(discovery): JWT flow #1059

Conversation

andrewazores commented Aug 25, 2022 • edited

maxcao13 left a comment

Choose a reason for hiding this comment

maxcao13 left a comment • edited

Choose a reason for hiding this comment

andrewazores commented Sep 6, 2022

andrewazores commented Sep 6, 2022 • edited

maxcao13 commented Sep 6, 2022 • edited

andrewazores commented Sep 6, 2022

andrewazores commented Sep 6, 2022

maxcao13 commented Sep 7, 2022

andrewazores commented Sep 8, 2022

maxcao13 left a comment

Choose a reason for hiding this comment

andrewazores commented Sep 12, 2022

andrewazores commented Sep 12, 2022

andrewazores commented Aug 25, 2022 •

edited

maxcao13 left a comment •

edited

andrewazores commented Sep 6, 2022 •

edited

maxcao13 commented Sep 6, 2022 •

edited