unverbuggt / mkdocs-encryptcontent-plugin

A MkDocs plugin that encrypt/decrypt markdown content with AES
https://unverbuggt.github.io/mkdocs-encryptcontent-plugin/
MIT License
123 stars 15 forks source link

Add Github secret option #26

Closed ListIndexOutOfRange closed 2 years ago

ListIndexOutOfRange commented 2 years ago

This implements a very minor yet powerful change:

Using Github actions workflow, it is possible to define a secret password, then make it accessible as an environment variable that the plugin can use instead of writing the expected password directly in the mkdocs.yml file. The added description in the README should be pretty straightforward to follow.

Note that this requires a requirements.txt file in order for the Github worker to build the docs.

Note also that I added some minor changes such as flake8 compliance with smaller max line length (100) and a .gitignore file. Thoses changes are obviously not required.

CoinK0in commented 2 years ago

Hi,

Thanks for the PR :)

I 100% agree with the idea to integrate a global password entry via the env.

However, I would like to "normalize" 3 things with your PR.

1- The true|false variable with "PASSWORD" by default which is used for this feature. I think there must be some environments where this variable could already be used. I would like to use your "use_secret" variable to, if set, target the "name" of the environment variable to be used. This would allow the user to choose the variable name themselves. It may also be necessary to add a control of this variable which could (by accident, error) not be defined and lead to the accidental revelation of the encrypted articles. Stopping the build process in this case makes sense to me.

2- The README part, which is very detailed/explicit. My opinion it's "a bit too much" and very focused on git actions, while some will probably want to do the same with more classic CI/CD pipelines (Jenkins for example). In order to keep a README as concise as possible, it seems to me more appropriate to only explain how to use of the environment variable, without describing the configuration, setting up a specific case.

3 - The creation of a "requirement.txt" with hard fixed versions. Probably I should define the compatible versions a little better in my setup.py, but the requirement proposed here is extremely related to your integration environment. This could lead to forcing other users to use these versions (undesirable).

Finally, i don't like formatting lines at 100 char max, it's more complicated to read, but who reads the code?! So I would do with. Thanks for the scan and corrections. :)

Do you want to make the changes yourself and redo a PR. Or change the destination branch to "experimental" (which I sync with master now) so that I can make the changes and create a 2.2.0 version?

Thanks for your contribution :)

ListIndexOutOfRange commented 2 years ago

Hi !

Thanks for your quick reply ! I can do the changes myself and redo a PR. Let me just process your answers to be sure I got it right.

1.a use_secret: boolean to string: Yes that is obviously a lot better. It's embarrassing I didn't think of that haha.

1.b use_secret being a potential risk: I'm not sure what you mean ? In which context could it happen ? Let's say I update the lines 157 & 158 of plugin.py to

if self.config['use_secret'] is not None:
    self.config['global_password'] = os.environ[self.config['use_secret']]

How could that be a security issue ? If the secret name doesn't match the use_secret value, that would throw a KeyError because I'm not using the os.environ.get() method. I agree that this not a good behavior though, and stopping the build process would be better. How would you implement that without requiring anything from the user CI/CD pipeline ?

2. README being to detailed: I kinda agree but I'm not sure. Do you suggest to only say something like "an environment variable named as use_secret must be set" ? I feel like many users could be interested in using this features without being familiar with any form of CI/CD (I'm a deep learning PhD student, and I can tell you it's a field where lots of people may want to secure their docs without knowing anything about Jenkins or Github actions). That's why i thought something like a minimal working example could be valuable.

3. "requirement.txt" with hard fixed versions: Yes that's completely my bad, I have some auto API reference generation from docstrings that seem to be pretty sensible to versions, I just forgot to change it before submitting, sorry. How would you like it ? No requirements at all, or loosen up the version, in which case I'm not sure which minimal versions are acceptable ?

4. max line length: My screen is in portrait format, that's why it was triggering me haha. Do you want me to revert it ? That's no big deal.

Let me know what you think about all that, and I'll do the PR asap.

Thanks !

CoinK0in commented 2 years ago

Sorry for the late reply. To answer your questions :

1b: Using the GET is good for me, it avoids the KeyError indeed. The "problem" is that if the "use_secret" variable is defined in the config BUT it is not correctly assigned in the env of the machine, the get() returns a "None" type and just after we use this "None" to set the "password". This will lead to the articles not being encrypted.

I would therefore continue to use the .get(), but I would just add a check on the return of the get.

# Set global password as default password for each page
if self.config.get('use_secret') and self.config['use_secret'] is not None:
    if os.environ.get(str(self.config['use_secret'])) is not None:
        self.config['global_password'] = os.environ.get(str(self.config['use_secret']))
    else:
        logger.error('Cannot get global password from environment variable: {}. Abort !'.format(str(self.config['use_secret'])))
        os.exit()
# Set global password as default password for each page
self.config['password'] = self.config['global_password']
  1. I understand the idea and it is true that it would surely be useful to someone. But in this case why describe Git Workflow Action and not something else . From my point of view it 's like if in the documentation of my microwave, I found described the shape of the dishes to be used .. It's necessary to give the restrictive information on what works or not, but the shape ... More However, here it's useful to explain how the env variable works, how to define it (the background), but not the form which may vary according to the use case and the technologies. I'd rather someone open an issue asking how to do it in specific case.

  2. I think it's better to not put requirement.txt. Some CI/CD will need this information for setup but everyone is free to do it as they wish. The right way to do this would be to modify my setup.py, which contains information on the dependencies to be installed for this plugin to work. Like that a pip install of the plugins would go by itself to look for the compatible versions. Honestly currently I don't know which version works or not, because I only test a few each time (manually).

  3. As you want ^^

Thanks.