Project

General

Profile

Actions

Bug #18676

closed

[api] handle anonymous token like system root token, removing need for db record

Added by Ward Vandewege 5 months ago. Updated 3 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
02/11/2022
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Description

As per the configuration reference:

      # Set AnonymousUserToken to enable anonymous user access. Populate this
      # field with a long random string. Then run "bundle exec
      # ./script/get_anonymous_user_token.rb" in the directory where your API
      # server is running to record the token in the database.
      AnonymousUserToken: "" 

The salt-based installer does not run that ruby script, which means the anonymous user token is unusable. Observed on 2.3-release.

It would be nice to do away with the need to run this script to make the anonymous token work.

Tom suggests: I suspect we could handle the anonymous token the same way we handle system root token, in app/models/api_client_authorization.rb -> check_system_root_token ... no db record or rake task needed. It would change the semantics a bit (changing the config would invalidate the old token, if you wanted multiple anon tokens you'd need to create real db records) which I think would be an improvement.

TODO:
  • make the rails adjustment as Tom suggested
  • update documentation
  • remove script/get_anonymous_user_token.rb and any remaining references to it

Subtasks 1 (0 open1 closed)

Task #18720: review 18676-anon-token-improvementResolvedWard Vandewege02/11/2022

Actions

Related issues

Related to Arvados - Feature #17298: remove the need to run get_anonymous_user_token.rb during installationResolved

Actions
Related to Arvados - Bug #18887: [federation] wb1 fiddlesticks in login federationResolvedWard Vandewege03/25/2022

Actions
Actions #1

Updated by Ward Vandewege 5 months ago

  • Description updated (diff)
Actions #2

Updated by Ward Vandewege 5 months ago

This get_anonymous_user_token.rb script is very old and spits out a lot of warnings on Ruby 2.7 (see below).

I don't know that there is a reason for it to exist anymore; we should just have controller read the anonymous token from the config on startup and if it does not exist in the database, create the appropriate record. That way the installer does not need to know/do anything.

# RAILS_ENV=production bundle exec ./script/get_anonymous_user_token.rb 
Defaulting to memory cache, because /var/www/arvados-api/current/tmp/cache does not exist
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/type.rb:27: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/type/adapter_specific_registry.rb:9: warning: The called method `add_modifier' is defined here
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activemodel-5.2.6/lib/active_model/type/integer.rb:13: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activemodel-5.2.6/lib/active_model/type/value.rb:8: warning: The called method `initialize' is defined here
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/connection_adapters/postgresql/oid/specialized_string.rb:12: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activemodel-5.2.6/lib/active_model/type/value.rb:8: warning: The called method `initialize' is defined here
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/actionpack-5.2.6/lib/action_dispatch/middleware/stack.rb:37: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/actionpack-5.2.6/lib/action_dispatch/middleware/ssl.rb:59: warning: The called method `initialize' is defined here
/var/www/arvados-api/current/app/models/container_request.rb:16: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/associations.rb:1653: warning: The called method `belongs_to' is defined here
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/type/adapter_specific_registry.rb:21: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/type/adapter_specific_registry.rb:42: warning: The called method `matches?' is defined here
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/type/adapter_specific_registry.rb:21: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/type/adapter_specific_registry.rb:109: warning: The called method `matches?' is defined here
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activemodel-5.2.6/lib/active_model/type/registry.rb:20: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/type/adapter_specific_registry.rb:34: warning: The called method `call' is defined here
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activesupport-5.2.6/lib/active_support/cache.rb:445: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activesupport-5.2.6/lib/active_support/cache.rb:716: warning: The called method `initialize' is defined here
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/transactions.rb:212: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/connection_adapters/abstract/database_statements.rb:260: warning: The called method `transaction' is defined here
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/connection_adapters/abstract/transaction.rb:171: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/connection_adapters/abstract/transaction.rb:97: warning: The called method `initialize' is defined here
v2/ac2it-gj3su-23wysa5b2pq3whf/uepahsheibai2aiqu0aeX0wiurevae3he1waucae6Gohv1phaeyey5xoiBuakeih
Actions #3

Updated by Peter Amstutz 5 months ago

  • Target version set to Kanban
  • Project changed from Arvados to 40
Actions #4

Updated by Ward Vandewege 5 months ago

  • Assigned To set to Ward Vandewege
Actions #5

Updated by Ward Vandewege 5 months ago

  • Target version changed from Kanban to 2022-02-16 sprint
  • Status changed from New to In Progress
  • Project changed from 40 to Arvados
Actions #6

Updated by Ward Vandewege 5 months ago

  • Subject changed from [installer] does not record anonymous user token in the database to [api] handle anonymous token like system root token, removing need for db record
Actions #7

Updated by Ward Vandewege 5 months ago

  • Description updated (diff)
Actions #8

Updated by Ward Vandewege 5 months ago

  • Related to Feature #17298: remove the need to run get_anonymous_user_token.rb during installation added
Actions #9

Updated by Ward Vandewege 5 months ago

Ready for review at ad2851bce9be401f8feac6570b3958ce93732cfd on branch 18676-anon-token-improvement

Test run: which has the usual flakiness in the wb1 integration tests, re-running at

Actions #10

Updated by Tom Clegg 5 months ago

It looks like a v2 token would not be accepted with the current code. I don't know of a specific case where that would come up, but it seems like it could happen easily, and we could accommodate it easily since it's literally "v2/#{Rails.configuration.ClusterID}-gj3su-anonymouspublic/#{Rails.configuration.Users.AnonymousUserToken}"...

It looks like the configured token will be silently ignored if it's less than 50 chars, which could be confusing for someone who upgraded without noticing the upgrade note / checking their token length. Could we make this a config-check warning instead (maybe add a mandatory bool arg to (*Loader)checkToken() in source:lib/config/load.go) so RailsAPI only has to check whether length>0? And maybe the minimum recommended length should be 32 like the other tokens, and we get the "invalid chars" check for free? (Technically I think minimum length of 1 would be no less secure, but consistency/simplicity is worth something.)

Super nitpicky: although "and" is (was?) more Ruby-fashionable, "&&" has the precedence one expects ("a = b && c" means "a = (b && c) but "a = b and c" means "(a = b) and c") ... IMO it is a better habit to use "&&" in logical expressions. I also find it easier to read, fwiw.

Rest LGTM. Hurray for deleting setup steps, especially rake tasks!

Actions #11

Updated by Ward Vandewege 5 months ago

Tom Clegg wrote:

It looks like a v2 token would not be accepted with the current code. I don't know of a specific case where that would come up, but it seems like it could happen easily, and we could accommodate it easily since it's literally "v2/#{Rails.configuration.ClusterID}-gj3su-anonymouspublic/#{Rails.configuration.Users.AnonymousUserToken}"...

True. I wonder if we need to make this change now? Putting an entire proper V2 token in the config file has never worked afaik.

It looks like the configured token will be silently ignored if it's less than 50 chars, which could be confusing for someone who upgraded without noticing the upgrade note / checking their token length. Could we make this a config-check warning instead (maybe add a mandatory bool arg to (*Loader)checkToken() in source:lib/config/load.go) so RailsAPI only has to check whether length>0? And maybe the minimum recommended length should be 32 like the other tokens, and we get the "invalid chars" check for free? (Technically I think minimum length of 1 would be no less secure, but consistency/simplicity is worth something.)

Good idea, I've made that change, thanks.

Super nitpicky: although "and" is (was?) more Ruby-fashionable, "&&" has the precedence one expects ("a = b && c" means "a = (b && c) but "a = b and c" means "(a = b) and c") ... IMO it is a better habit to use "&&" in logical expressions. I also find it easier to read, fwiw.

OK! Fixed.

Ready for another look at 053f74285455278bed87cd4b3dc0df2adffb3b9c

Tests at
, wb1 integration tests re-run at

Actions #12

Updated by Tom Clegg 5 months ago

The boolean expressions in checkToken() seem a bit obfuscated -- (mandatory || len(token) > 0) is just a long way to say len(token) > 0 because the first clause returns early if (mandatory && len(token) 0) -- maybe it would be clearer to put "if !mandatory && token "" { return nil } at the top?

True. I wonder if we need to make this change now? Putting an entire proper V2 token in the config file has never worked afaik.

I'm thinking either we make the change now, or we wait for someone/ourselves to fall into the trap, troubleshoot, and file a bug report. Seems like less trouble to fix it now.

To clarify, I meant we should preserve the existing behavior of accepting "Authorization: Bearer v2/.../{secret}" from API clients as a equivalent to "Authorization: Bearer {secret}" ... I wasn't thinking about having a v2 token in the config file.

Rest LGTM

Actions #13

Updated by Ward Vandewege 5 months ago

Tom Clegg wrote:

The boolean expressions in checkToken() seem a bit obfuscated -- (mandatory || len(token) > 0) is just a long way to say len(token) > 0 because the first clause returns early if (mandatory && len(token) 0) -- maybe it would be clearer to put "if !mandatory && token "" { return nil } at the top?

Yes, of course, I've simplified it that way.

True. I wonder if we need to make this change now? Putting an entire proper V2 token in the config file has never worked afaik.

I'm thinking either we make the change now, or we wait for someone/ourselves to fall into the trap, troubleshoot, and file a bug report. Seems like less trouble to fix it now.

To clarify, I meant we should preserve the existing behavior of accepting "Authorization: Bearer v2/.../{secret}" from API clients as a equivalent to "Authorization: Bearer {secret}" ... I wasn't thinking about having a v2 token in the config file.

We had a chat about this - the code already handles V2 tokens but an additional check that the token uuid matches the anon token UUID is advised. I've made that change.

Rest LGTM

629cd91ffca67d6de5ad4dbe9854a064f9e26820 on branch 18676-anon-token-improvement

Tests at

Actions #14

Updated by Tom Clegg 5 months ago

LGTM, thanks!

Actions #15

Updated by Ward Vandewege 5 months ago

  • % Done changed from 0 to 100
  • Status changed from In Progress to Resolved
Actions #16

Updated by Ward Vandewege 5 months ago

  • Target version changed from 2022-02-16 sprint to 2022-03-02 sprint
  • Status changed from Resolved to In Progress

Re-opening this: it turns out that the `script/get_anonymous_user_token.rb` command was printing out fully formed v2 tokens, which means that the value for Users/AnonymousUserToken is likely to be a full v2 token in existing installations.

This means we need to

  • be able to parse a v2 token properly (ignoring the part before the secret, since we now have a standard uuid for the anonymous user token)
  • turn the config-check error into a warning when a full v2 token is provided in the config file
  • add a mention of this in the release notes

d6c1841ea8d87238fa18a673fb524985e826ae19 on branch 18676-anon-token-support-v2-in-config

Tests at

Actions #17

Updated by Tom Clegg 5 months ago

Couple of suggestions
  • If the configured token happens to contain two "/" but doesn't start with "v2/", use the generic "bad chars" error instead of "token is a full V2 token"
  • I suppose we should continue accepting the literal v2 token that was in the config file, even though the UUID is not the new predictable one (in case it has been copied & pasted into some client). Presumably we would do this by accepting any uuid with the right cluster id and infix?
Actions #18

Updated by Ward Vandewege 5 months ago

Tom Clegg wrote:

Couple of suggestions
  • If the configured token happens to contain two "/" but doesn't start with "v2/", use the generic "bad chars" error instead of "token is a full V2 token"

Sure, done.

  • I suppose we should continue accepting the literal v2 token that was in the config file, even though the UUID is not the new predictable one (in case it has been copied & pasted into some client). Presumably we would do this by accepting any uuid with the right cluster id and infix?

I don't think that's needed - if a full v2 token is the config file on upgrade, a corresponding database record will exist (or, it never worked!), and that would still work, right? It just wouldn't use the new automatic anonymous token code path.

63645c871246a61a2148b259f10d2fedf30e8df8 on branch 18676-anon-token-support-v2-in-config

Tests at

Actions #19

Updated by Tom Clegg 5 months ago

if strings.Index(token, "v2/") == -1 should probably be if strings.Index(token, "v2/") != 0 ... or if !strings.HasPrefix(token, "v2/")

  • I suppose we should continue accepting the literal v2 token that was in the config file, even though the UUID is not the new predictable one (in case it has been copied & pasted into some client). Presumably we would do this by accepting any uuid with the right cluster id and infix?

I don't think that's needed - if a full v2 token is the config file on upgrade, a corresponding database record will exist (or, it never worked!), and that would still work, right? It just wouldn't use the new automatic anonymous token code path.

Oh, that's right. I forgot the existing db records would still exist.

But that reminds me -- I think we need to add scopes: ["GET /"] to the new implementation.

Actions #20

Updated by Ward Vandewege 4 months ago

Tom Clegg wrote:

if strings.Index(token, "v2/") == -1 should probably be if strings.Index(token, "v2/") != 0 ... or if !strings.HasPrefix(token, "v2/")

Ah, right, that's more precise. Fixed.

  • I suppose we should continue accepting the literal v2 token that was in the config file, even though the UUID is not the new predictable one (in case it has been copied & pasted into some client). Presumably we would do this by accepting any uuid with the right cluster id and infix?

I don't think that's needed - if a full v2 token is the config file on upgrade, a corresponding database record will exist (or, it never worked!), and that would still work, right? It just wouldn't use the new automatic anonymous token code path.

Oh, that's right. I forgot the existing db records would still exist.

But that reminds me -- I think we need to add scopes: ["GET /"] to the new implementation.

Yes! Added.

608e8f79c3fb5cb7077fce4a0b497c5c93d6d6d0 on branch 18676-anon-token-support-v2-in-config

Tests at

re-running api server tests which had a git checkout problem:

Actions #21

Updated by Tom Clegg 4 months ago

LGTM, thanks!

Actions #22

Updated by Ward Vandewege 4 months ago

  • Status changed from In Progress to Resolved
Actions #23

Updated by Peter Amstutz 3 months ago

  • Release set to 46
Actions #24

Updated by Ward Vandewege 3 months ago

  • Related to Bug #18887: [federation] wb1 fiddlesticks in login federation added
Actions

Also available in: Atom PDF