Skip to content
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

Jump to bottom

Fixes #25903: Refactor API tokens after clear-text removal #6031

Open
wants to merge 4 commits into
base: branches/rudder/8.3
Choose a base branch
from
Open

Fixes #25903: Refactor API tokens after clear-text removal #6031

wants to merge 4 commits into from
+405 −200

Conversation

amousset
Copy link
Member

@amousset amousset commented Nov 19, 2024 •
edited
Loading

https://issues.rudder.io/issues/25903

A similar change needs to be done in the api-authorizations plugins for user tokens.

Backend

Data types

Split the ApiToken type, which used to contain both clear-text and hashed values into two separate types to prevent confusions and prevent misusage as much as possible:

  • ApiTokenSecret: The token value, to be sent to the creator and not stored on the server
  • ApiTokenHash: The token hash, as stored (either in LDAP or memory)
    • Make no effort to prepare for a v3 (appart from storage), it would only be needed in case sha2 is not considered safe anymore. Future changes to the API access would likely be for something quite different (OAUTH/JWT/...), with a different architecture.

Also modifying the account types:

  • ApiAccount now contains a ApiTokenHash
  • A new NewApiAccount is created, to be used after creation, and contains a ApiTokenSecret

Notes:

  • The system token now also uses a hash, and the clear text value is not kept after the token files creation.
  • Make the access to the secret and hash values explicit with expose() methods (and make the value private)

Usage

The JSON serialization of the accounts, used by the API tokens Web interface, is modified:

  • For new accounts, the token value is sent
  • For existing account, the token hash version only is sent

Frontend

  • Display the list of unsupported tokens along with a global warning
  • Hide the enable/disable button for unsupported tokens (as it could not be actionnable)

image

@amousset amousset requested a review from fanf November 19, 2024 21:03
@amousset amousset marked this pull request as draft November 19, 2024 21:03
@amousset amousset force-pushed the arch_25903/refactor_api_tokens_after_clear_text_removal branch from ef47b5e to 3d3c090 Compare November 20, 2024 00:32
@amousset
Copy link
Member Author

Commit modified

@amousset amousset force-pushed the arch_25903/refactor_api_tokens_after_clear_text_removal branch from 3d3c090 to 5c706c4 Compare November 20, 2024 00:57
@amousset
Copy link
Member Author

Commit modified

@amousset amousset force-pushed the arch_25903/refactor_api_tokens_after_clear_text_removal branch from 5c706c4 to d060cfb Compare November 20, 2024 02:23
@amousset
Copy link
Member Author

Commit modified

@amousset amousset force-pushed the arch_25903/refactor_api_tokens_after_clear_text_removal branch from d060cfb to e729750 Compare November 20, 2024 03:34
@amousset
Copy link
Member Author

Commit modified

@amousset
Copy link
Member Author

Commit modified

@amousset amousset force-pushed the arch_25903/refactor_api_tokens_after_clear_text_removal branch from e729750 to 2057a85 Compare November 20, 2024 07:10
@amousset amousset marked this pull request as ready for review November 20, 2024 08:06
@amousset amousset requested review from clarktsiory and removed request for fanf November 22, 2024 09:07
@amousset
fixup! Fixes #25903: Refactor API tokens after clear-text removal
59937dc 
Fixes #25903: Refactor API tokens after clear-text removal
@amousset
Copy link
Member Author

PR updated with a new commit

@amousset
fixup! fixup! Fixes #25903: Refactor API tokens after clear-text removal
8224c52 
Fixes #25903: Refactor API tokens after clear-text removal
@amousset
Copy link
Member Author

PR updated with a new commit

clarktsiory
clarktsiory reviewed Nov 26, 2024
View reviewed changes
Copy link
Contributor

@clarktsiory clarktsiory left a comment

Choose a reason for hiding this comment

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

Great change !
There only is some compilation problems

...der-core/src/main/scala/com/normation/rudder/services/marshalling/XmlSerialisationImpl.scala
@@ -536,7 +536,7 @@ class APIAccountSerialisationImpl(xmlVersion: String) extends APIAccountSerialis
(
<id>{account.id.value}</id>
<name>{account.name.value}</name>
<token>{account.token.value}</token>
<token>{account.token.exposeHash()}</token>
Copy link
Contributor

Choose a reason for hiding this comment

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

Since in an Option is returned, it will write Some("thehash"), and this would break. It should be a String, so .getOrElse("")

Copy link
Contributor

Choose a reason for hiding this comment

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

It compiles now but this will cause a problem without the getOrElse : https://scastie.scala-lang.org/h2QlFY3BSzqlEBruNceAvA

webapp/sources/rudder/rudder-rest/src/main/scala/com/normation/rudder/users/User.scala Outdated Show resolved Hide resolved
webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/api/DataStructures.scala Show resolved Hide resolved
webapp/sources/rudder/rudder-web/src/main/elm/sources/Accounts/JsonDecoder.elm Show resolved Hide resolved
@amousset
fixup! fixup! fixup! Fixes #25903: Refactor API tokens after clear-te…
1ee29e5 
…xt removal

Fixes #25903: Refactor API tokens after clear-text removal
@amousset
Copy link
Member Author

PR updated with a new commit

fanf
fanf reviewed Nov 28, 2024
View reviewed changes
...udder/rudder-core/src/main/scala/com/normation/rudder/repository/ldap/LDAPEntityMapper.scala
@@ -1142,7 +1142,7 @@ class LDAPEntityMapper(
mod.resetValuesTo(A_API_UUID, principal.id.value)
mod.resetValuesTo(A_NAME, principal.name.value)
mod.resetValuesTo(A_CREATION_DATETIME, GeneralizedTime(principal.creationDate).toString)
mod.resetValuesTo(A_API_TOKEN, principal.token.exposeHash())
mod.resetValuesTo(A_API_TOKEN, principal.token.exposeHash().getOrElse(""))
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that will work. The API token is a mandatory field in the schema for apiAccount.
Perhaps we want to be able to have that case, but it seems to be a hard hypothesis in the lower layout. We can perhaps workaround it with a marker value that would be extracted as none.

Copy link
Member

Choose a reason for hiding this comment

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

or even v2:

@amousset amousset changed the base branch from master to branches/rudder/8.3 January 10, 2025 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clarktsiory clarktsiory clarktsiory left review comments

@fanf fanf fanf left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@amousset @fanf @clarktsiory