KNOX-2554 - Implemented JDBC Token State Service #433

smolnar82 · 2021-04-13T19:56:08Z

What changes were proposed in this pull request?

This change introduces a new TokenStateService implementation that operates using pure JDBC connections to relational databases. In the first version, only Postgres and Derby database types are supported.

How was this patch tested?

Added new JUnit test classes and updated existing test cases as needed.

I also executed manual testing:

Used our new tokengen application to fetch a token

2021-04-19 23:24:38,972 DEBUG knox.gateway (GatewayFilter.java:doFilter(116)) - Received request: GET /knoxtoken/api/v1/token
2021-04-19 23:24:38,982 INFO  service.knoxtoken (TokenResource.java:getAuthenticationToken(419)) - Knox Token service (homepage) issued token eyJhbG...WdNDTw (8c419cf4...675b1dc14cce)
2021-04-19 23:24:39,001 DEBUG token.state (JDBCTokenStateService.java:addToken(59)) - Token 8c419cf4...675b1dc14cce has been saved in the database
2021-04-19 23:24:39,002 DEBUG token.state (DefaultTokenStateService.java:addToken(144)) - Added token 8c419cf4...675b1dc14cce, expiration 2021-05-19T21:24:38.978Z
2021-04-19 23:24:39,011 DEBUG token.state (JDBCTokenStateService.java:addMetadata(186)) - Updated metadata for 8c419cf4...675b1dc14cce in the database
2021-04-19 23:24:39,012 DEBUG service.knoxtoken (TokenResource.java:getAuthenticationToken(446)) - Knox Token service (homepage) stored state for token eyJhbG...WdNDTw (8c419cf4...675b1dc14cce)

postgres=# select * FROM KNOX_TOKENS where token_id LIKE '8c419cf4%';
               token_id               |  issue_time   |  expiration   | max_lifetime  | username | comment 
--------------------------------------+---------------+---------------+---------------+----------+---------
 8c419cf4-ff57-4412-93dd-675b1dc14cce | 1618867478983 | 1621459478978 | 1619472278983 | admin    | 
(1 row)

Renewed the previously acquired token:

$ curl -ku admin:admin-password -d "@token.txt" -X POST https://localhost:8443/gateway/sandbox/knoxtoken/api/v1/token/renew
{
  "renewed": "true",
  "expires": "1618954035727"
}

2021-04-19 23:27:15,711 DEBUG knox.gateway (GatewayFilter.java:doFilter(116)) - Received request: POST /knoxtoken/api/v1/token/renew
2021-04-19 23:27:15,713 INFO  knox.gateway (KnoxLdapRealm.java:getUserDn(688)) - Computed userDn: uid=admin,ou=people,dc=hadoop,dc=apache,dc=org using dnTemplate for principal: admin
2021-04-19 23:27:15,740 DEBUG token.state (JDBCTokenStateService.java:updateExpiration(108)) - Updated expiration for 8c419cf4...675b1dc14cce in the database to 1,618,954,035,727
2021-04-19 23:27:15,740 DEBUG token.state (DefaultTokenStateService.java:renewToken(219)) - Renewed token 8c419cf4...675b1dc14cce, expiration 2021-04-20T21:27:15.727Z
2021-04-19 23:27:15,741 INFO  service.knoxtoken (TokenResource.java:renew(280)) - Knox Token service (sandbox) renewed the expiration for token eyJhbG...WdNDTw (8c419cf4...675b1dc14cce) (renewer=admin)

postgres=# select * FROM KNOX_TOKENS where token_id LIKE '8c419cf4%';
               token_id               |  issue_time   |  expiration   | max_lifetime  | username | comment 
--------------------------------------+---------------+---------------+---------------+----------+---------
 8c419cf4-ff57-4412-93dd-675b1dc14cce | 1618867478983 | 1618954035727 | 1619472278983 | admin    | 
(1 row)

Used that token to list WEBHDFS status. This time the configured WEBHDFS backend pointed to a non-existing cluster, but the point is that the supplied Passcode token could be validated. I also covered this case by restarting Knox and repeated the same step, so that a DB lookup was necessary as the token was not available in the in-memory collections:

$ curl -ku Passcode:8c419cf4-ff57-4412-93dd-675b1dc14cce https://localhost:8443/gateway/tokenbased/webhdfs/v1?op=LISTSTATUS
<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=utf-8"/>
<title>Error 500 java.io.IOException: Service connectivity error.</title>
</head>
...
</html>


2021-04-19 23:32:49,216 DEBUG knox.gateway (GatewayFilter.java:doFilter(116)) - Received request: GET /webhdfs/v1
2021-04-19 23:32:49,336 DEBUG token.state (JDBCTokenStateService.java:getTokenExpiration(90)) - Fetched expiration for 8c419cf4...675b1dc14cce from the database : 1,618,954,035,727
2021-04-19 23:32:49,343 DEBUG token.state (JDBCTokenStateService.java:getTokenMetadata(215)) - Fetched metadata for 8c419cf4...675b1dc14cce from the database

postgres=# select * FROM KNOX_TOKENS where token_id LIKE '8c419cf4%';
               token_id               |  issue_time   |  expiration   | max_lifetime  | username | comment 
--------------------------------------+---------------+---------------+---------------+----------+---------
 8c419cf4-ff57-4412-93dd-675b1dc14cce | 1618867478983 | 1618954035727 | 1619472278983 | admin    | 
(1 row)

Revoked that token:

$ curl -ku admin:admin-password -d "@token.txt" -X POST https://localhost:8443/gateway/sandbox/knoxtoken/api/v1/token/revoke
{
  "revoked": "true"
}


2021-04-19 23:34:36,081 DEBUG knox.gateway (GatewayFilter.java:doFilter(116)) - Received request: POST /knoxtoken/api/v1/token/revoke
2021-04-19 23:34:36,124 INFO  knox.gateway (KnoxLdapRealm.java:getUserDn(688)) - Computed userDn: uid=admin,ou=people,dc=hadoop,dc=apache,dc=org using dnTemplate for principal: admin
2021-04-19 23:34:36,808 DEBUG token.state (DefaultTokenStateService.java:removeTokenState(290)) - Removed state for tokens 8c419cf4...675b1dc14cce
2021-04-19 23:34:36,808 DEBUG token.state (JDBCTokenStateService.java:removeToken(159)) - Token 8c419cf4...675b1dc14cce has been removed from the database
2021-04-19 23:34:36,809 DEBUG token.state (DefaultTokenStateService.java:revokeToken(244)) - Revoked token 8c419cf4...675b1dc14cce
2021-04-19 23:34:36,809 INFO  service.knoxtoken (TokenResource.java:revoke(327)) - Knox Token service (sandbox) revoked token eyJhbG...WdNDTw (8c419cf4...675b1dc14cce) (renewer=admin)

postgres=# select * FROM KNOX_TOKENS where token_id LIKE '8c419cf4%';
 token_id | issue_time | expiration | max_lifetime | username | comment 
----------+------------+------------+--------------+----------+---------
(0 rows)

I also let the Knox Gateway running for the long weekend and confirmed that expired tokens got removed by the reaper thread.

moresandeep

Looks great Sandor!!
I did a quick review and noted few observations. Also, synchronization can be issue here, might want to consider using CompletableFutures for optimization, failures and thread safety.

...-server/src/main/java/org/apache/knox/gateway/services/token/impl/JDBCTokenStateService.java

...way-server/src/main/java/org/apache/knox/gateway/services/token/impl/TokenStateDatabase.java

moresandeep · 2021-04-13T20:14:06Z

...way-server/src/main/java/org/apache/knox/gateway/services/token/impl/TokenStateDatabase.java

+public class TokenStateDatabase {
+  private static final String TOKENS_TABLE_NAME = "KNOX_TOKENS";
+  private static final String ADD_TOKEN_SQL = "INSERT INTO " + TOKENS_TABLE_NAME + "(token_id, issue_time, expiration, max_lifetime) VALUES(?, ?, ?, ?)";
+  private static final String REMOVE_TOKENS_SQL_PREFIX = "DELETE FROM " + TOKENS_TABLE_NAME + " WHERE token_id IN (";


Just a thought, why not list them under resources as a *.sql statements so all of them are in one place and easy to manage and look at.

I'm not sure if I follow. Do you think that having a .sql file is better than having all token-related SQL statements in one place (in this DAO class)?

I have seen this implemented in other projects where SQL was used heavily. The idea being all SQL stays in one resource place and if you need to tweak the SQL statements you don't have to worry about changing the java files. Even better if you can have them as external resources you could change them at runtime without changing class files (but that is not what I am expecting just something that came to me while writing :) )
I am not opposed to having it in DAO but my thinking is SQLs statements are more like resources.

I found this article on SO. Let me quote the pros/cons of my approach and I do agree with them:

Hardcoded/encapsulated in DAO layer Pros: SQL is kept in the objects that access data (encapsulation) SQL is easy to write (speed of development) SQL is easy to track down when changes are required A simple solution (no messy architecture) Cons: SQL cannot be reviewed/changed by DBA SQL is likely to become DB-specific SQL can become hard to maintain

Given the fact we are going to keep authentication tokens in this DB table, I have the impression that these SQL statements should reside in the Java class so that they cannot be changed/manipulated/removed easily.

At my previous workplaces, where pure JDBC was used, no ORM (Hibernate, EclipseLink, etc) frameworks, we followed this approach because of the above-listed pros.
There was an exception where lots of business logic was written in PL/SQL but - to be honest - I really hated that part.

smolnar82

Thanks, @moresandeep !
I'm going to submit a new patch soon with the getTokenIds optimization and the removal locking the rest should be ok IMO.

...-server/src/main/java/org/apache/knox/gateway/services/token/impl/JDBCTokenStateService.java

smolnar82 · 2021-04-14T07:24:28Z

...way-server/src/main/java/org/apache/knox/gateway/services/token/impl/TokenStateDatabase.java

+public class TokenStateDatabase {
+  private static final String TOKENS_TABLE_NAME = "KNOX_TOKENS";
+  private static final String ADD_TOKEN_SQL = "INSERT INTO " + TOKENS_TABLE_NAME + "(token_id, issue_time, expiration, max_lifetime) VALUES(?, ?, ?, ?)";
+  private static final String REMOVE_TOKENS_SQL_PREFIX = "DELETE FROM " + TOKENS_TABLE_NAME + " WHERE token_id IN (";


I'm not sure if I follow. Do you think that having a .sql file is better than having all token-related SQL statements in one place (in this DAO class)?

...way-server/src/main/java/org/apache/knox/gateway/services/token/impl/TokenStateDatabase.java

…statement

smolnar82 · 2021-04-14T08:25:12Z

@moresandeep - thanks again for your review. In fact, I made it even more clean and effective by overriding the evictExpiredTokens from the parent class. This way a simple DELETE statement is enough.

…es are executed from the public API

gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java

lmccay

Looks good, added one review comment.

...-server/src/main/java/org/apache/knox/gateway/services/token/impl/JDBCTokenStateService.java

smolnar82 · 2021-04-20T06:37:34Z

Merging based on @lmccay 's comment: this change looks good except for the suggested refactoring which I created a JIRA for.

This commit does not contain secrets Change-Id: I12152e2ce24d46bd6f8254127fc9e3634044b081

* changes: CDPD-25489 - Added missing pieces from previous Apache commits CDPD-25489 KNOX-2596 - Changed gateway-site config value to support PostgreSQL as token state backend (apache#439) CDPD-25489 KNOX-2595 - KNOX_TOKENS table is created automatically if not exists (apache#438) CDPD-25489 KNOX-2554 - Implemented JDBC Token State Service (apache#433)

KNOX-2554 - Implemented JDBC Token State Service

14d6b28

smolnar82 requested review from lmccay, moresandeep and pzampino April 13, 2021 19:56

smolnar82 self-assigned this Apr 13, 2021

smolnar82 added the knoxtoken label Apr 13, 2021

moresandeep reviewed Apr 13, 2021

View reviewed changes

smolnar82 commented Apr 14, 2021

View reviewed changes

KNOX-2554 - Removing expired tokens from the DB with a simple DELETE …

4cec238

…statement

smolnar82 requested a review from moresandeep April 14, 2021 08:23

smolnar82 added 2 commits April 14, 2021 10:34

KNOX-2554 - Update in-memory collections only if DB operation succeeded

8612736

KNOX-2554 - Throwing a runtime exception from methods where DML queri…

b146e78

…es are executed from the public API

moresandeep reviewed Apr 14, 2021

View reviewed changes

gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java Show resolved Hide resolved

smolnar82 requested a review from moresandeep April 14, 2021 13:59

smolnar82 force-pushed the KNOX-2554 branch from ad46a12 to d7d0bb8 Compare April 14, 2021 21:41

KNOX-2554 - Added JUnit tests and implemented missing methods

19583d5

smolnar82 force-pushed the KNOX-2554 branch from d7d0bb8 to 19583d5 Compare April 14, 2021 22:21

KNOX-2554 - Fixed wrong remove token method

31c2298

lmccay requested changes Apr 19, 2021

View reviewed changes

...-server/src/main/java/org/apache/knox/gateway/services/token/impl/JDBCTokenStateService.java Show resolved Hide resolved

smolnar82 merged commit b07dbee into apache:master Apr 20, 2021

smolnar82 deleted the KNOX-2554 branch April 20, 2021 06:37

stoty pushed a commit to stoty/knox that referenced this pull request May 14, 2024

CDPD-25489 KNOX-2554 - Implemented JDBC Token State Service (apache#433)

f08f74c

This commit does not contain secrets Change-Id: I12152e2ce24d46bd6f8254127fc9e3634044b081

KNOX-2554 - Implemented JDBC Token State Service #433

KNOX-2554 - Implemented JDBC Token State Service #433

Uh oh!

Conversation

smolnar82 commented Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

moresandeep left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

moresandeep Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

smolnar82 Apr 14, 2021

Choose a reason for hiding this comment

Uh oh!

moresandeep Apr 14, 2021

Choose a reason for hiding this comment

Uh oh!

smolnar82 Apr 14, 2021

Choose a reason for hiding this comment

Uh oh!

smolnar82 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smolnar82 Apr 14, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

smolnar82 commented Apr 14, 2021

Uh oh!

Uh oh!

lmccay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smolnar82 commented Apr 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

smolnar82 commented Apr 13, 2021 •

edited

Loading