Skip to content

update at_api_keys table to allow for a key_id column #135

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
adamkorynta wants to merge 2 commits into
base: main
Choose a base branch
from
Open

update at_api_keys table to allow for a key_id column #135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1bc8b16
update at_api_keys table to allow for a key_id column
adamkorynta Apr 3, 2026
773b77a
remove set_session_user_apikey as CDA uses set_session_user_direct
adamkorynta Apr 3, 2026
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
9 changes: 0 additions & 9 deletions schema/src/cwms/cwms_env_pkg.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,6 @@ AS
* @parameter p_office The office for a particular session.
*/
PROCEDURE set_session_user_direct(p_user VARCHAR2, p_office VARCHAR2 default NULL);

/**
* For user by WEB_USER accounts, set the role given the provided apikey
*
* @parameter p_apikey A user authorization token with a user defined (default 1 day) lifetime.
* @parameter p_office The office required for a particular session.
*
*/
PROCEDURE set_session_user_apikey(p_apikey VARCHAR2, p_office VARCHAR2 default NULL);
END cwms_env;
/

Expand Down
8 changes: 0 additions & 8 deletions schema/src/cwms/cwms_env_pkg_body.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,14 +166,6 @@ AS
raise;
END set_session_user_direct;

PROCEDURE set_session_user_apikey(p_apikey VARCHAR2, p_office VARCHAR2)
IS
l_userid at_sec_cwms_users.userid%type := null;
BEGIN
select userid into l_userid from cwms_20.av_active_api_keys where apikey = p_apikey;
set_session_user_direct(l_userid,p_office);
end set_session_user_apikey;

PROCEDURE set_session_privileges
IS
l_office_id VARCHAR2 (16);
Expand Down
21 changes: 11 additions & 10 deletions schema/src/cwms/tables/at_api_keys.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,26 @@
create table at_api_keys(
key_id raw(16) not null,
userid varchar2(128) not null references at_sec_cwms_users(USERID),
key_name varchar2(64) not null,
apikey varchar2(256) not null unique,
created date default current_timestamp not null,
expires date default current_timestamp+1,
primary key(userid,key_name)
secret_hash varchar2(512) not null,
created timestamp default current_timestamp not null,
expires timestamp default current_timestamp+1,
constraint at_api_keys_pk primary key (key_id),
Copy link
Copy Markdown
Contributor

@MikeNeilson MikeNeilson Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's with key_id? I don't see it used anywhere. Seems like composite key of userid, key_name should be sufficient as a primary key.

Copy link
Copy Markdown
Contributor Author

@adamkorynta adamkorynta Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to be able to send an id in plain text (or encoded) as part of the apikey in order for quick lookup. Using userid would be escaping sensitive data, and there's no protection or guidance on key_name to know whether users are escaping sensitive data there either.

Copy link
Copy Markdown
Contributor

@MikeNeilson MikeNeilson Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I just read the cda PR and that makes sense. I put a though on the topic there.

Though I wonder if having the key ultimately be provided as key_name:key might be the way to go. As you said, there's no other way. and if we do that, then you can just pull the hashed key down and make use of password4j's update mechanism if parameters change. As you said, we'd need to provide some guidance and constraints if we do that.

Copy link
Copy Markdown
Contributor Author

@adamkorynta adamkorynta Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we only do key_name then we'd have to index on key_name for faster search and I don't think we really want key_name to be unique - or if we're actually storing key_name:key then we'd index on secret_hash and we'd be doing a where secret_hash like 'abc123.%' to filter the results. Adding a raw(16) key_id column would be faster and would be a smaller index.

Copy link
Copy Markdown
Contributor

@MikeNeilson MikeNeilson Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indexes don't have to be unique indexes. We could still keep the user_id/keyname index which keeps the search space smaller.

While it is conceptually possible for two argon2 hashes to be the same, because of the use of the salt it is like something 10^-<rather large number> probability. and we could put the user_id as a prefix into the stored hash for that chance and then just extract it from the string before passing it into the checker.

Copy link
Copy Markdown
Contributor Author

@adamkorynta adamkorynta Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, there are other options if we force the user to provide more information

Copy link
Copy Markdown
Contributor

@MikeNeilson MikeNeilson Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow - we can't extract the user_id from the hash

I was originally thinking "combine them", e.g. "<user_id>_" but realized it was non-sensical and never went back to correct the thought.

Copy link
Copy Markdown
Contributor

@MikeNeilson MikeNeilson Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So thinking about this discussion, I think I've realized why S3 auth is access_key + access_secret.

So if we keep key_id, but randomly generate it and then encode it to text somehow, and provide that as a "username".

the new maven central api does the same thing. To simplify (on the CDA side) perhaps we can output the apikey column as <key_id>: so that everyone still just copy pastes it and we don't need to update the DTO.

Copy link
Copy Markdown
Contributor Author

@adamkorynta adamkorynta Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're on the same page now. My intention was to return the "apikey" as ak1_<encoded_key_id>:<actual_key>. Then on read we'd strip out ak1_ as the version, <encoded_key_id> decoded to match on the key_id column, and then argon2 hash check the stored hash against the apikey. <encoded_key_id> would be randomly generated and fixed size just like the apikey currently is.

Copy link
Copy Markdown
Contributor

@MikeNeilson MikeNeilson Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. Otherwise we're just guessing which key is what, even if the collision odds are exceptionally low.

constraint at_api_keys_u1 unique (userid, key_name)
);

comment on column at_api_keys.apikey is
'While randomly generated, these still have to be unique. Applications generating them should check and provide a different value';

create or replace trigger cwms_20.st_api_key_readonly
before update on cwms_20.at_api_keys
for each row
begin
if( :new.userid <> :old.userid
if( (:new.userid <> :old.userid
OR :new.key_name <> :old.key_name
OR :new.apikey <> :old.apikey
OR :new.secret_hash <> :old.secret_hash
OR :new.created <> :old.created
OR :new.key_id <> :old.key_id)
AND :new.expires < :old.expires
) then
raise_application_error(-20001,'API Key table is unmodifiabled except to update expiration or by deletion.');
raise_application_error(-20001,'Only EXPIRES may be updated; all other columns are immutable.');
end if;
end;
/
Expand Down
3 changes: 2 additions & 1 deletion schema/src/cwms/views/av_active_api_keys.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
create or replace view av_active_api_keys as
select
key_id,
userid,
key_name,
apikey,
secret_hash,
created,
expires
from
Expand Down
36 changes: 4 additions & 32 deletions schema/src/test/test_webuser_abilities.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,45 +118,17 @@ AS

procedure can_interact_with_api_keys_table_and_view is
l_userid varchar(128) := upper('&eroc.hectest');
l_testkey cwms_20.at_api_keys.apikey%type := 'A simple test key';
l_testkey cwms_20.at_api_keys.secret_hash%type := 'A simple test key';
l_testkey_name cwms_20.at_api_keys.key_name%type := 'A test key';
l_testkey_name_out cwms_20.at_api_keys.key_name%type;
begin
cwms_20.cwms_env.set_session_user_direct('&&eroc.webtest','&&office_id');
insert into cwms_20.at_api_keys(userid,key_name,apikey)
values (l_userid,l_testkey_name,l_testkey);
select key_name into l_testkey_name_out from cwms_20.av_active_api_keys where apikey=l_testkey;
insert into cwms_20.at_api_keys(key_id,userid,key_name,secret_hash)
values (secret_hash, l_userid,l_testkey_name,l_testkey);
select key_name into l_testkey_name_out from cwms_20.av_active_api_keys where secret_hash=l_testkey;
ut.expect(l_testkey_name_out).to_equal(l_testkey_name);
end;


procedure can_set_context_user_by_key is
l_testkey1 cwms_20.at_api_keys.apikey%type := 'User 1 Test Key';
l_testkey2 cwms_20.at_api_keys.apikey%type := 'User 2 Test Key';
l_user1 varchar2(128) := upper('&eroc.hectest');
l_user2 varchar2(128) := upper('&eroc.hectest_ro');
l_priv varchar2(255);
begin
insert into cwms_20.at_api_keys(userid,key_name,apikey)
values (l_user1,l_testkey1,l_testkey1);
insert into cwms_20.at_api_keys(userid,key_name,apikey)
values (l_user2,l_testkey2,l_testkey2);

cwms_env.set_session_user_apikey(l_testkey1,'&&office_id');
ut.expect(cwms_util.get_user_id).to_equal(l_user1);

-- test without office ID set
cwms_env.set_session_user_apikey(l_testkey2);
ut.expect(cwms_util.get_user_id).to_equal(l_user2);
ut.expect(SYS_CONTEXT ('CWMS_ENV', 'CWMS_PRIVILEGE')).to_equal('READ_ONLY');

/** I don't believe it but this actually is required, which is good. But
we should still call it to make sure it doesn't fail.
*/
cwms_env.set_session_user_direct(upper('&eroc.webtest'),'&&office_id');

end;

PROCEDURE test_create_cwms_cwbi_user
IS
l_count number;
Expand Down
Loading