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

feat: SSL certificate for DAPI #519

Merged
merged 21 commits into from
Sep 16, 2022
Merged

feat: SSL certificate for DAPI #519

merged 21 commits into from
Sep 16, 2022

Conversation

shuplenkov
Copy link
Collaborator

@shuplenkov shuplenkov commented Sep 5, 2022 •
edited
Loading

Issue being fixed or feature implemented

Need to obtain SSL certificate during setup command execution

What was done?

Dashmate setup now obtains SSL certificate by itself

How Has This Been Tested?

Manual

Breaking Changes

No

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

@shuplenkov shuplenkov added this to the v0.24.0 milestone Sep 5, 2022
@lgtm-com
Copy link

lgtm-com bot commented Sep 5, 2022

This pull request introduces 1 alert when merging 57099de into 853f140 - view on LGTM.com

new alerts:

  • 1 for Disabling certificate validation

@lgtm-com
Copy link

lgtm-com bot commented Sep 5, 2022

This pull request introduces 1 alert when merging fe2e598 into 853f140 - view on LGTM.com

new alerts:

  • 1 for Disabling certificate validation

@lgtm-com
Copy link

lgtm-com bot commented Sep 5, 2022

This pull request introduces 1 alert when merging d12286c into 853f140 - view on LGTM.com

new alerts:

  • 1 for Disabling certificate validation

@lgtm-com
Copy link

lgtm-com bot commented Sep 5, 2022

This pull request introduces 1 alert when merging ece720b into 853f140 - view on LGTM.com

new alerts:

  • 1 for Disabling certificate validation

@shumkov shumkov changed the title feat(dashmate): setup command obtains certificate feat: SSL certificate for DAPI in dashmate Sep 5, 2022
@shumkov shumkov changed the title feat: SSL certificate for DAPI in dashmate feat: SSL certificate for DAPI Sep 5, 2022
shumkov
shumkov requested changes Sep 5, 2022
View reviewed changes
packages/dapi-grpc/clients/platform/v0/nodejs/PlatformPromiseClient.js Outdated Show resolved Hide resolved
packages/dashmate/configs/schema/configJsonSchema.js Outdated Show resolved Hide resolved
packages/dashmate/configs/schema/configJsonSchema.js Outdated
enum: ['zerossl', 'manual'],
},
zerossl: {
type: ['object', 'null'],
Copy link
Member

Choose a reason for hiding this comment

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

Remove type null

packages/dashmate/configs/schema/configJsonSchema.js Outdated Show resolved Hide resolved
packages/dashmate/configs/schema/configJsonSchema.js Outdated Show resolved Hide resolved
packages/dashmate/templates/platform/drive/tenderdash/config.toml.dot Outdated Show resolved Hide resolved
packages/js-dapi-client/lib/dapiAddressProvider/DAPIAddress.js Outdated Show resolved Hide resolved
packages/js-dapi-client/lib/transport/GrpcTransport/GrpcTransport.js Outdated Show resolved Hide resolved
packages/platform-test-suite/karma.conf.js Outdated Show resolved Hide resolved
packages/platform-test-suite/lib/test/getDAPISeeds.js Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Sep 12, 2022

This pull request introduces 1 alert when merging 3822ab9 into 853f140 - view on LGTM.com

new alerts:

  • 1 for Disabling certificate validation

@lgtm-com
Copy link

lgtm-com bot commented Sep 12, 2022

This pull request introduces 1 alert when merging a0fb44c into 853f140 - view on LGTM.com

new alerts:

  • 1 for Disabling certificate validation

@lgtm-com
Copy link

lgtm-com bot commented Sep 15, 2022

This pull request introduces 23 alerts and fixes 13 when merging 4ef3433 into 853f140 - view on LGTM.com

new alerts:

  • 13 for Unused variable, import, function or class
  • 8 for Useless assignment to local variable
  • 1 for Useless conditional
  • 1 for Disabling certificate validation

fixed alerts:

  • 10 for Expression has no effect
  • 2 for Useless assignment to local variable
  • 1 for Unneeded defensive code

@lgtm-com
Copy link

lgtm-com bot commented Sep 15, 2022

This pull request introduces 23 alerts and fixes 13 when merging 5d89bf8 into 853f140 - view on LGTM.com

new alerts:

  • 13 for Unused variable, import, function or class
  • 8 for Useless assignment to local variable
  • 1 for Useless conditional
  • 1 for Disabling certificate validation

fixed alerts:

  • 10 for Expression has no effect
  • 2 for Useless assignment to local variable
  • 1 for Unneeded defensive code

@lgtm-com
Copy link

lgtm-com bot commented Sep 16, 2022

This pull request introduces 1 alert when merging 9c299ca into 853f140 - view on LGTM.com

new alerts:

  • 1 for Disabling certificate validation

@lgtm-com
Copy link

lgtm-com bot commented Sep 16, 2022

This pull request introduces 1 alert when merging afdf5bf into 853f140 - view on LGTM.com

new alerts:

  • 1 for Disabling certificate validation

shumkov
shumkov approved these changes Sep 16, 2022
View reviewed changes
Copy link
Member

@shumkov shumkov left a comment

Choose a reason for hiding this comment

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

👍

@shumkov shumkov merged commit f992d92 into v0.24-dev Sep 16, 2022
@shumkov shumkov deleted the dashmate-ssl branch September 16, 2022 15:17
@shumkov shumkov mentioned this pull request Sep 30, 2022
Closed
5 tasks
coolaj86
coolaj86 reviewed Sep 30, 2022
View reviewed changes
packages/dashmate/src/ssl/generateKeyPair.js
@@ -0,0 +1,16 @@
const forge = require('node-forge');
Copy link

@coolaj86 coolaj86 Sep 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

forge is outdated and very slow (not suitable for Raspberry Pi and such). Consider using the node crypto and acme (the original native Let's Encrypt module for node):

These are fast, lightweight, and use native crypto.

Copy link
Member

Choose a reason for hiding this comment

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

Good point, thank you. @shuplenkov please update it your current PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coolaj86 coolaj86 coolaj86 left review comments

@shumkov shumkov shumkov approved these changes

@antouhou antouhou Awaiting requested review from antouhou

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.24.0
Development

Successfully merging this pull request may close these issues.
3 participants
@shuplenkov @shumkov @coolaj86