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 #18972: API to use secret database #3534

Closed
wants to merge 13 commits into from
Closed

Fixes #18972: API to use secret database #3534

wants to merge 13 commits into from

Conversation

ElaadF
Copy link
Member

@ElaadF ElaadF commented Mar 10, 2021

@ElaadF ElaadF force-pushed the ust_18972/api_to_use_secret_databse branch from af4ba0e to 9a812c4 Compare March 10, 2021 10:07
@ElaadF
Copy link
Member Author

ElaadF commented Mar 10, 2021

Commit modified

@VinceMacBuche
Copy link
Member

I think you should rename secretVault into secret or secrets depending on the case. I don't to make think it could be related to hashicorp Vault

@ElaadF
Copy link
Member Author

ElaadF commented Mar 10, 2021

Currently, Delete and Update actions doesn't return an error if the secret doesn't exists

@ElaadF ElaadF added the WIP Use that label for a Work In Progress PR that must not be merged yet label Mar 10, 2021
@ElaadF ElaadF requested a review from fanf March 10, 2021 10:10
VinceMacBuche
VinceMacBuche reviewed Mar 10, 2021
View reviewed changes
...rudder/rudder-rest/src/main/scala/com/normation/rudder/web/services/SecretVaultService.scala Outdated

import java.nio.charset.StandardCharsets

final case class Metadata(author: Option[String], formatVersion: Option[String], date: Option[String])
Copy link
Member

Choose a reason for hiding this comment

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

You don't need those metadata, author and date

format should not be optionnal, and i think it should not be a parameter

Copy link
Member Author

Choose a reason for hiding this comment

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

@fanf mentionned that it could be interesting to have extra metadas, but nothing for the moment, I can remove it, should we keep formatVersion anyway ?

@ElaadF
Merge branch 'ust_18972/api_to_use_secret_databse' of github.com:Elaa…
aabd75c 
…dF/rudder into ust_18972/api_to_use_secret_database
@ElaadF
fixup! Merge branch 'ust_18972/api_to_use_secret_databse' of github.c…
8c9dd9f 
…om:ElaadF/rudder into ust_18972/api_to_use_secret_database

Fixes #18972: API to use secret database
@ElaadF
Copy link
Member Author

ElaadF commented Mar 10, 2021

Commit modified

@ElaadF ElaadF force-pushed the ust_18972/api_to_use_secret_databse branch from 0a0ce76 to d2bfa92 Compare March 10, 2021 17:51
@ElaadF
Copy link
Member Author

ElaadF commented Mar 10, 2021

Commit modified

@ElaadF ElaadF force-pushed the ust_18972/api_to_use_secret_databse branch from d2bfa92 to 8478a27 Compare March 10, 2021 17:54
VinceMacBuche
VinceMacBuche reviewed Mar 24, 2021
View reviewed changes
webapp/sources/rudder/rudder-rest/src/main/scala/com/normation/rudder/rest/lift/SecretApi.scala Outdated
JString(id)
}

RestUtils.response(restExtractor, "secretName", None)(res, req, "Error when trying to delete a secret")
Copy link
Member

Choose a reason for hiding this comment

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

you should always send back the whole secret structure with the same container than the rest of the api

VinceMacBuche
VinceMacBuche reviewed Mar 24, 2021
View reviewed changes
webapp/sources/rudder/rudder-rest/src/main/scala/com/normation/rudder/rest/lift/SecretApi.scala Outdated
Secret.serializeSecret(secret)
}

RestUtils.response(restExtractor, "secret", None)(res, req, "Error when trying to add a secret")
Copy link
Member

Choose a reason for hiding this comment

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

should be secrets and in an array

VinceMacBuche
VinceMacBuche reviewed Mar 24, 2021
View reviewed changes
webapp/sources/rudder/rudder-rest/src/main/scala/com/normation/rudder/rest/lift/SecretApi.scala Outdated
Secret.serializeSecret(secret)
}

RestUtils.response(restExtractor, "secret", None)(res, req, s"Error when trying to update a secret")
Copy link
Member

Choose a reason for hiding this comment

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

Should be secrets and in an array

VinceMacBuche
VinceMacBuche reviewed Mar 24, 2021
View reviewed changes
.../rudder-rest/src/main/scala/com/normation/rudder/web/services/EventLogDetailsGenerator.scala Outdated
</ul>
{(
"#name" #> modDiff.name &
"#value" #> mapSimpleDiff(modDiff.modValue)
Copy link
Member

Choose a reason for hiding this comment

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

Should state that value has changed and (diff strucure should only have a boolean, stating it has changed or not

VinceMacBuche
VinceMacBuche reviewed Mar 24, 2021
View reviewed changes
...rces/rudder/rudder-rest/src/main/scala/com/normation/rudder/web/services/SecretService.scala Outdated
_ <- oldSecret match {
case Some(oldSec) =>
if(oldSec.value == newSecret.value) {
logger.warn(s"Trying to update secret `${oldSec.name}` with the same value")
Copy link
Member

Choose a reason for hiding this comment

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

log level should not be warn and it don't even think we need to log it

VinceMacBuche
VinceMacBuche reviewed Mar 24, 2021
View reviewed changes
...rces/rudder/rudder-rest/src/main/scala/com/normation/rudder/web/services/SecretService.scala Outdated
}

class FileSystemSecretRepository(
jsonDbPath : String
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be configuration-repository path, and the end of the path should be in the repository

btw, the secrets.json should be in a secrets directory

@ElaadF
Copy link
Member Author

ElaadF commented Apr 9, 2021

PR updated with a new commit
add description parameter, standardize API return format and remove value from eventlog

ElaadF
ElaadF commented Apr 9, 2021
View reviewed changes
...r-core/src/main/scala/com/normation/rudder/services/marshalling/XmlUnserialisationImpl.scala
// value <- (secret \ "value").headOption.map( _.text ) ?~! ("Missing attribute 'value' in entry type secret : " + entry)
description <- (secret \ "description").headOption.map( _.text ) ?~! ("Missing attribute 'description' in entry type secret : " + entry)
} yield {
Secret(name, "", description)
Copy link
Member Author

@ElaadF ElaadF Apr 9, 2021 •
edited

Choose a reason for hiding this comment

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

Not the best solution, maybe create a SecretInfo(name: String, description: String) and used it instead of Secret
and transform Secret(name: String, value: String, description: String) into Secret(value: String, info: SecretInfo)

@ElaadF ElaadF force-pushed the ust_18972/api_to_use_secret_databse branch from b4e6438 to 690f3bb Compare April 9, 2021 04:22
@ElaadF
Copy link
Member Author

ElaadF commented Apr 9, 2021

Commit modified

@ElaadF ElaadF force-pushed the ust_18972/api_to_use_secret_databse branch from 690f3bb to 9a98782 Compare April 9, 2021 07:44
@ElaadF
Copy link
Member Author

ElaadF commented Apr 9, 2021

Commit modified

@ElaadF
fixup! Fixes #18972: API to use secret database
01d227f 
Fixes #18972: API to use secret database
@ElaadF
Copy link
Member Author

ElaadF commented Apr 9, 2021

PR updated with a new commit
adding support of git

@ElaadF
fixup! fixup! Fixes #18972: API to use secret database
39ff9be 
Fixes #18972: API to use secret database
@ElaadF
Copy link
Member Author

ElaadF commented Apr 11, 2021

PR updated with a new commit
optionnal field when empty for update API and make mandatory all field when adding new secret

@ElaadF
Copy link
Member Author

ElaadF commented Jul 28, 2021

This PR should not be in rudder main project, it will be moved in rudder-plugins

@ElaadF ElaadF closed this Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fanf fanf Awaiting requested review from fanf

@VinceMacBuche VinceMacBuche Awaiting requested review from VinceMacBuche

Assignees
No one assigned
Labels
Trigger test WIP Use that label for a Work In Progress PR that must not be merged yet
Projects
None yet
Milestone
No milestone
3 participants
@ElaadF @VinceMacBuche @fanf