Skip to content
This repository has been archived by the owner on Mar 28, 2023. It is now read-only.

[WIP] Start of unit test for gencerts subcommand: #1241

Closed
wants to merge 6 commits into from

Conversation

allen-munsch
Copy link
Contributor

@allen-munsch allen-munsch commented Oct 20, 2018

#949

  • WIP for testing: produces valid and usable certificates
  • generates the certificates in the expected location

However I was confused by args :

func (x *GenerateCertificates) Execute(args []string) error {

I didn't understand how args was being used, sort of looked like it wasn't being used.

As far as interesting use cases, i'm not sure how interesting the current settings are.

It'd be awesome if i could get pointed to some use cases that I could test for, if they exist somewhere.

 - WIP for testing: produces valid and usable certificates
 - generates the certificates in the expected location
@coveralls
Copy link

coveralls commented Oct 20, 2018

Coverage Status

Coverage remained the same at 33.694% when pulling c4353fc on allen-munsch:am_949 into cda68e2 on OpenBazaar:master.

@cpacia
Copy link
Member

cpacia commented Oct 20, 2018

Thanks for this. I think the number one use case is for getting the electron UI client to make a valid ssl connection to the server.

The trouble we had was programatically generating a cert that electron would accept. I believe the current code does that but not 100% sure. If we could even some how import it into electron rather than the OS level that would be OK too.

But remote connecting client to server is the use case.

Copy link
Member

@placer14 placer14 left a comment

Choose a reason for hiding this comment

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

Thanks for the unit test. A few changes requested but good over all! 🍻

cmd/gencerts_test.go Outdated Show resolved Hide resolved
cmd/gencerts_test.go Outdated Show resolved Hide resolved
cmd/gencerts_test.go Outdated Show resolved Hide resolved
cmd/gencerts_test.go Show resolved Hide resolved
cmd/gencerts_test.go Outdated Show resolved Hide resolved
@allen-munsch
Copy link
Contributor Author

@placer14 thank you for the feedback I think I made the suggested changes.

As far as certificate import this is what i've found so far, however i'm not sure if the nssdb method is outdated, or if its cross platform.

win32/64

  certutil.exe -addstore -user Root openbazaard.crt

darwin

  security add-trusted-cert -p ssl openbazaard.crt

linux

  certutil -d sql:$HOME/.pki/nssdb -A -t "C,," -n 127.0.0.1 -i openbazaard.pem

A rough sketch of ssl configuration through electron, but only with linux support.

  if (process.platform === 'linux') {
    let sslcert = {
                   certificate: `${config.sslcert.path}`,
                   password: `${config.sslcert.password}`
                  }
    app.importCertificate(sslcert, function(result) {
      if (result != 0) {
        // https://cs.chromium.org/chromium/src/net/base/net_error_list.h
        dialog.showErrorBox('Error importing ssl certificate',
          `Error number: ${}\nCertificate path: ${config.sslcert.path}`)
      }
    })
  })

I'm still looking into how openbazaar-go and openbazaar-desktop do ssl. To better understand the test case.

Copy link
Member

@placer14 placer14 left a comment

Choose a reason for hiding this comment

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

One more round of changes, @allen-munsch. Thank you for spending your time on this. And apologies about the slow response on the review.

cmd/gencerts_test.go Outdated Show resolved Hide resolved
cmd/gencerts_test.go Outdated Show resolved Hide resolved
cmd/gencerts_test.go Show resolved Hide resolved
cmd/gencerts_test.go Outdated Show resolved Hide resolved
cmd/gencerts_test.go Show resolved Hide resolved
cmd/gencerts_test.go Outdated Show resolved Hide resolved
cmd/gencerts_test.go Outdated Show resolved Hide resolved
cmd/gencerts_test.go Outdated Show resolved Hide resolved
@allen-munsch
Copy link
Contributor Author

@placer14 thank you very much for the review. I've been super busy, but i'm hoping to get to these changes soon. Just a personal factor, but i'm on a low bandwidth connection, like 20Gb allotment, and it get's eaten up pretty quick using openbazaar. recently switched machines and dependency downloads with 70kb connection are difficult, so i've been waiting. lol.

@placer14
Copy link
Member

@allen-munsch I'm really sorry to hear that. Unfortunately, our dependency management is still using godep and is not kind on the bandwidth.

We've had a few people complaining about SSL not working properly. Seeing that you're already familiar with this area of the code, maybe you'd like to take a look at it? You could use your current versions to test/explore as well. #1307

@placer14 placer14 changed the title Start of unit test for gencerts subcommand: [WIP] Start of unit test for gencerts subcommand: Dec 10, 2018
@cpacia
Copy link
Member

cpacia commented May 24, 2019

Reopening as #1602 in an ob-go branch

@cpacia cpacia closed this May 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants