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

Added command for generating certs #124

Merged

Conversation

eschricker
Copy link
Contributor

Description

A new command was added, that can be used for generating certificates and updating the env-file.
Common code between the command for generating secrets and certs has been refactored into a trait.

Checklist:

  • I've added tests for my changes or tests are not applicable
  • I've changed documentations or changes are not required
  • I've added my changes to CHANGELOG.md

@eschricker eschricker self-assigned this Feb 22, 2022
@eschricker
Copy link
Contributor Author

@mfn @leon0399 would you like to review the PR? Or should we merge @Messhias ?

@Messhias
Copy link
Collaborator

Messhias commented Mar 7, 2022

@mfn @leon0399 would you like to review the PR? Or should we merge @Messhias ?

let's merge, they've already approved it.

@Messhias Messhias merged commit c25239f into PHP-Open-Source-Saver:main Mar 7, 2022
Copy link
Contributor

@mfn mfn left a comment

Choose a reason for hiding this comment

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

A bit late to the part 😅

Very nice PR!

Left some minor feedback. Is there a chance to write some basic tests?

Comment on lines +75 to +78
// check if laravel version Less than 5.4.17
if (version_compare($this->laravel->version(), '5.4.17', '<')) {
return $this->laravel->basePath() . DIRECTORY_SEPARATOR . '.env';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed, we only support L6+

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed this in #145

Comment on lines +30 to +34
static $filepath = null;

if(is_null($filepath)) {
$filepath = $this->envPath();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a benefit to the static? I think we can just call $this->envPath() all the time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed this in #145

@@ -54,3 +54,39 @@ This will update your `.env` file with something like `JWT_SECRET=foobar`

It is the key that will be used to sign your tokens. How that happens exactly will depend
on the algorithm that you choose to use.

### Generate certificate
Copy link
Contributor

Choose a reason for hiding this comment

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

The instruction are identical to the laravel-installation.md, aren't they?

Can we make a dedicated section for this somewhere, without duplicating this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed this in #146


// save certificates to disk
if (false === is_dir($directory)) {
mkdir($directory);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should consider making it recursively to improve DX:

             mkdir($directory, 0777, true);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed this in #146

$algo = $this->option('algo') ? $this->option('algo') : 'rsa';
$bits = $this->option('bits') ? intval($this->option('bits')) : 4096;
$shaVariant = $this->option('sha') ? intval($this->option('sha')) : 512;
$passphrase = $this->option('passphrase') ? $this->option('passphrase') : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if providing the passphrase via command line is a good idea (think about shell history file).

Maybe it should be possible to ask() for this too?

E.g. --passphrase without providing one would ask for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed this in #146

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.

None yet

3 participants