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.

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

Recipe Secrets Section #6421

Closed
wants to merge 7 commits into from
Closed

Conversation

deanmarcussen
Copy link
Member

@deanmarcussen deanmarcussen commented Jun 14, 2020

Edited and refactored comments below

This introduces the idea of a Properties section in a recipe / deployment plan.

The idea is that it provides a standard way of configuring 'special' properties, such as passwords, so that recipes / deployment plans can still use them, in an appropriate context.

I've done this for the SmtpSettings.

A setting which should be protected, such as the smtp password, now has 4 options to choose from when creating a deployment step.

  • UserSupplied - the property will be created and the user is expected to complete this value manually
  • PlainText - the password will be decrypted and stored as plain text in the plan. This option will only work if you are transfering server to server. It will error if you try and export it to a file.
  • Encrypted - the property will be stored in the recipe as an encrypted value. To work on another server the data protection key will have to be present on both servers (i.e. use dataprotection.azure and push both servers to the same blob store.)
  • Ignored - the property will be completely ignored (which stops it being overwritten if it's already been set on the target server.
  "tags": [],
  "properties": {
    "SmtpSettings": {
      "Password": {
        "Handler": "UserSupplied",
        "Value": ""
      }
    }
  },
  "steps": [
    {
      "name": "SmtpSettings",
      "SmtpSettings": {
        "DefaultSender": "foo@test.com",
        ... removed for brevity ...
      }
    }
  ]

The reason for introducing a properties section here, is that it later, gives us the ability to parse this section, and provide in the ui, or in a setup context, a form, for the user to actually complete these values as part of the setup process.
This would only show the UserSupplied properties, for example.

It also allows you (oob) to extract this properties section, and cut and paste it directly into your appsettings.Development.json file under the section OrchardCore_Recipes.

The idea behind this is to be able to configure development secrets, via usersecrets, or environment variables, so that the properties are all in one place and reasonably visible.

SelectList
SmtpSettings

/cc @Piedone

@agriffard
Copy link
Member

Related to #5493

@deanmarcussen
Copy link
Member Author

#396

@deanmarcussen
Copy link
Member Author

Original design #5556

@deanmarcussen
Copy link
Member Author

Following discussion at Triage on Thursday, I think this has tried to mix the idea of properties/parameters and secrets together, when actually they are different (a little related), but still different issues.

So I will rework this to be specifically secret related, and we can look at parameters later.
Realistically we would need to build the ui for parameters at the same time.

@agriffard agriffard mentioned this pull request Jun 23, 2020
@JoshLefebvre
Copy link
Contributor

Not sure if in anyway related, but I recently submitted PR #6422 for a new OC module that uses Azure Key Vault to retrieve secrets from the vault. I use Key Vault heavily in my day to day to protect my passwords, connection strings etc.

@deanmarcussen
Copy link
Member Author

@JoshLefebvre definitely related, and partly why I included the ability to easily migrate / store these secrets in IConfiguration, which will then work with your keyvault pr. (which looks good, I think Seb just wants a look at it to)

I lost the thread, but I am interested, somewhere you commented that you would likely use the Encrypted setting. I am curious, as I thought that would be the least used, because people would have to keep data protection keys in sync across all tenants (or stored in the same blob storage, so effectively the same).

Do you plan to do this?

@JoshLefebvre
Copy link
Contributor

JoshLefebvre commented Jun 24, 2020

I suppose I only meant that of the options presented, the encrypted option seemed the most secure, therefore I would have been most likely to use this (provided there was a good example showing how to do so). However if I am able to read it through the IConfiguration at runtime then I can simply read from my KeyVault and never have to store it in plain text in my source code.

@deanmarcussen
Copy link
Member Author

I suppose I only meant that of the options presented, the encrypted option seemed the most secure, therefore I would have

Thanks, was just curious, as hoping I have covered all bases :)
I think IConfiguration will end up the easiest way for those of us that want to keep these secure. Encrypted will work, but would require sharing Data Protection Keys, so has it's own complications.

@deanmarcussen deanmarcussen changed the title Recipe Properties Section for secrets Recipe Secrets Section Jun 27, 2020
@deanmarcussen
Copy link
Member Author

Refactored to a secrets section, similar to parameters but not trying to combine the two concepts, as they are instrically different.

  "secrets": {
    "smtpPassword": {
      "Handler": "PlainText",
      "Value": "myPassword"
    }
  },
  "steps": [
    {
      "name": "SmtpSettings",
      "SmtpSettings": {
        "DefaultSender": "dean@test.com",
        "DeliveryMethod": 0,
        "PickupDirectoryLocation": null,
        "Host": "email.com",
        "Port": 25,
        "AutoSelectEncryption": false,
        "RequireCredentials": true,
        "UseDefaultCredentials": false,
        "EncryptionMethod": 2,
        "UserName": "myuser",
        "PasswordSecret": {
          "Handler": "[js: secretsHandler('smtpPassword')]",
          "Value": "[js: secrets('smtpPassword')]"
        }
      }
    }
  ]

So the values are stored in a secrets section, then the step is able to reference the secrets section via secretsHandler, and secrets and the secrets property name.

The step still needs to know how the secret is contstructed, so it can deal with how to data protect it.

A parameters section might look similar, but with very different metadata about how to construct the ui for the parameter.

UI options on the step now look like this
Screenshot 2020-06-27 at 11 43 50

  • Empty - the section is created, but no value supplied (so expecting the user to edit the recipe)
  • Encrypted - relies on data protection keys being in sync
  • PlainText - export in plain text, and user cannot export to a file, but can to a remote instance.
  • Ignored - don't export anything related to the password, for those who would just set the password themselves once on a remote instance, and use this step to keep the other settings in sync. (ultimately the most secure option)

Secrets section can still be moved directly to IConfiguration under the OrchardCore_Recipes section (or to a keyvault etc)

# Conflicts:
#	src/OrchardCore.Modules/OrchardCore.Email/OrchardCore.Email.csproj
#	src/OrchardCore/OrchardCore.Recipes.Core/Services/RecipeExecutor.cs
@agriffard
Copy link
Member

Will it be ready for 1.0?

@deanmarcussen
Copy link
Member Author

This pr to be replaced by the secrets module demo here https://www.youtube.com/watch?v=qPCBgHQYz1g

Maybe not ready for 1.0 will see how much time I have

@SzymonSel
Copy link
Member

Bumping this topic. Should this be merged?

@hishamco
Copy link
Member

Can you fix the conflict issue, so we can move on

@hishamco
Copy link
Member

There's no plan for this? what about Secret Module #7891?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants