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

Feature/myst 79 regen openvpn keys #150

Merged
merged 15 commits into from Feb 14, 2018

Conversation

Projects
None yet
4 participants
@zolia
Copy link
Member

commented Feb 7, 2018

No description provided.

"path/filepath"
)

// SecurityPrimitives describes security primitives

This comment has been minimized.

Copy link
@donce

donce Feb 8, 2018

Contributor

what is "security primitives"?

ServerCertPath string
ServerKeyPath string
CRLPEMPath string
TLSCryptKeyPath string

This comment has been minimized.

Copy link
@donce

donce Feb 8, 2018

Contributor

This list can be shortened by grouping coupled arguments, i.e.:

CACertPath, CAKeyPath string
ServerCertPath, ServerKeyPath string

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 8, 2018

Member

Or we could have several primitives?


// SecurityPrimitives describes security primitives
type SecurityPrimitives struct {
directory string

This comment has been minimized.

Copy link
@donce

donce Feb 8, 2018

Contributor

Since it's private, it might make sense to move it below public fields.

nil,
nil,
nil,
nil,

This comment has been minimized.

Copy link
@donce

donce Feb 8, 2018

Contributor

Very long list - since all params are given unnamed here, it's very easy to mismatch them. Especially since most of params are filenames.

&SecurityPrimitives{
  CACertPath: filepath.Join(genDir, caDir, "ca.crt"),
  CAKeyPath: filepath.Join(genDir, caDir, "ca.key"),
  ...
}
)

// SecurityPrimitives describes security primitives
type SecurityPrimitives struct {

This comment has been minimized.

Copy link
@donce

donce Feb 8, 2018

Contributor

Is this class "security primitives" itself, or does it generate "security primitives", i.e. "SecurityPrimitivesGenerator"?

}

// Generate generates required security primitives
func (sp *SecurityPrimitives) Generate() {

This comment has been minimized.

Copy link
@donce

donce Feb 8, 2018

Contributor

Since it's a public and most important method, it would be more readable if it higher in file - before private functions.

@@ -58,6 +59,10 @@ func NewCommandWith(
locationDetector = location.NewDetectorFake("")
}

// (Re)generate required security primitives before openvpn start
openVPNPrimitives := primitives.NewOpenVPNSecPrimitives()
openVPNPrimitives.Generate()

This comment has been minimized.

Copy link
@donce

donce Feb 8, 2018

Contributor

What happens if some certificate generation failed? When can this happen?

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 8, 2018

Member

Is it really necessary to have methods which modify state? Maybe we can have more functional style like primitives := GeneratePrimitives()

This comment has been minimized.

Copy link
@zolia

zolia Feb 8, 2018

Author Member

@tadovas , you might want to generate just a few of all primitives. Renamed Generate() to GenerateAll()

@@ -58,6 +59,10 @@ func NewCommandWith(
locationDetector = location.NewDetectorFake("")
}

// (Re)generate required security primitives before openvpn start

This comment has been minimized.

Copy link
@donce

donce Feb 8, 2018

Contributor

This factory having certificate generation inside is a bit unexpected - maybe this can be done in the command itself? We can initialize openVPNPrimitives here and pass it to Command as a parameter for dependency injection - this will make it more transparent.

ca := &x509.Certificate{
SerialNumber: big.NewInt(1653),
Subject: pkix.Name{
Country: []string{"Gibraltar"},

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 8, 2018

Member

These needs to be moved at least to const level first, maybe extracting this info as parameters

@tadovas
Copy link
Member

left a comment

IMHO you should totaly separate certificate generation from serialization (i.e. files) and maybe just make method persist( writer io.Writer) or something. The best way would be to have separate stateless functions for generation and maybe a separate serialization function/structure which can write itself to file

return nil, err
}

certOut, err := os.Create(p.CACertPath)

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 8, 2018

Member

Generating sertificate and writing to file is too much for this function. What about returning certificate structure which can later be serialized to any compatible destination?

cert := &x509.Certificate{
SerialNumber: big.NewInt(1658),
Subject: pkix.Name{
Country: []string{"Germany"},

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 8, 2018

Member

Same here - extract at least to constants

// generate sever cert / key
sp.CreateCert(ca, true)

if _, err := os.Stat(sp.ServerCertPath); os.IsNotExist(err) {

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 8, 2018

Member

If generator stops doing two things (generating AND persisting) tests will be much easier to write and much simplier. Tdd kicks in? :)

serverPrivateKey *ecdsa.PrivateKey
}

func exists(path string) (bool, error) {

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 8, 2018

Member

Nothing to do with primitives or generate, looks like os/filesystem layer to me

return true, err
}

func (sp *SecurityPrimitives) mkDir(dir string) {

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 8, 2018

Member

same here

commonDir = "common"
)

func (sp *SecurityPrimitives) init() {

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 8, 2018

Member

looks like "constructor" - don't make it separate init method

glide.lock Outdated
testImports:
- name: github.com/golang/lint
version: f635bddafc7154957bd70209ee858a4b97e64a9b
version: ""

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 8, 2018

Member

Why not freeze? :/

glide.lock Outdated
subpackages:
- encoders/builtin
- util
- name: github.com/nats-io/nuid

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 8, 2018

Member

Why? :O

@@ -34,17 +34,17 @@ func NewServerConfig(

func NewClientConfig(
remote string,
caFile, authFile string,
secPrimitives *primitives.SecurityPrimitives,

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 8, 2018

Member

Why to inject struct with 6fields, while we use only 2 of them

OrganizationalUnit: []string{"Mysterium Team"},
},
NotBefore: time.Now(),
NotAfter: time.Now().AddDate(10, 0, 0),

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 8, 2018

Member

Suggest to use readable durations with time.Now().Add(10 * time.Second)

This comment has been minimized.

Copy link
@zolia

zolia Feb 8, 2018

Author Member

there are no time.Year duration. No way to add years in such fashion.

return nil, err
}
pem.Encode(keyOut, pemBlockForKey(p.caPrivateKey))
keyOut.Close()

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 8, 2018

Member

defer key.OutClose()


func (sp *SecurityPrimitives) mkDir(dir string) {
if e, err := exists(dir); !e {
log.Debug("Creating dir (" + dir + ")")

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 8, 2018

Member

Unhandled errors

b, err := x509.MarshalECPrivateKey(k)
if err != nil {
fmt.Fprintf(os.Stderr, "Unable to marshal ECDSA private key: %v", err)
os.Exit(2)

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 8, 2018

Member

Dont panic

err := removeContents(sp.directory)
if err != nil {
fmt.Println(err)
os.Exit(1)

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 8, 2018

Member

Dont panic

taKey := make([]byte, 256)
_, err := rand.Read(taKey)
if err != nil {
fmt.Println("error:", err)

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 8, 2018

Member

Debug line?

}
}

log.Debug("written " + sp.TLSCryptKeyPath)

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 8, 2018

Member

Would be good to follow same logging as we to in other packages (prefix)

func (sp *SecurityPrimitives) init() {
sp.cleanupDir()

sp.mkDir(sp.directory)

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 9, 2018

Member

Dont create new directories, rather inject&use options.directoryRuntime

This comment has been minimized.

Copy link
@zolia

zolia Feb 9, 2018

Author Member

should I create everything in single directory, or should I create subdirectories still?

This comment has been minimized.

Copy link
@zolia

zolia Feb 9, 2018

Author Member

removing all new directories..

OrganizationalUnit: []string{"Mysterium Team"},
},
NotBefore: time.Now(),
NotAfter: time.Now().AddDate(10, 0, 0),

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 12, 2018

Member

Is it possible to have readable duration?

return &x509.Certificate{
SerialNumber: big.NewInt(1658),
Subject: pkix.Name{
Country: []string{"Germany"},

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 12, 2018

Member

Put node's country

SerialNumber: big.NewInt(1658),
Subject: pkix.Name{
Country: []string{"Germany"},
CommonName: "Mysterium Node",

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 12, 2018

Member

Put node's identity

return sp, err
}

const logPrefix = "[config-generator] "

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 12, 2018

Member

-> "[config-openvpn] "

sp.caPrivateKey = key

// generate sever cert / key
sp.createCert(ca, true)

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 12, 2018

Member

This test is testing checkCertificate, so certificates should be pre-created in Git already

@@ -55,8 +55,6 @@ imports:
version: 8727e98aa1b91610eb184ed1ab615943b8d9deb0
- name: github.com/pborman/uuid
version: e533369306653d193b93dae055f6083cbf8ba54f
- name: github.com/pkg/errors
version: 8842a6e0cc595d1cc9d931f6c875883967280e32

This comment has been minimized.

Copy link
@donce

donce Feb 12, 2018

Contributor

Nice catch! 👍

@@ -56,8 +56,8 @@ func (c *Config) SetTLSPrivatePubKeys(certFile string, certKeyFile string) {
c.AddOptions(OptionFile("key", certKeyFile))
}

func (c *Config) SetTlsAuth(authFile string) {
c.AddOptions(OptionFile("tls-auth", authFile))
func (c *Config) SetTlsCrypt(cryptFile string) {

This comment has been minimized.

Copy link
@donce

donce Feb 12, 2018

Contributor

SetTLSCrypt

@@ -41,6 +41,10 @@ func NewManagement(socketAddress, logPrefix string, middlewares ...ManagementMid
}
}

func (management *Management) SocketAddress() string {

This comment has been minimized.

Copy link
@donce

donce Feb 12, 2018

Contributor

Missing doc. There are quite a few other undocumented publics newly written in this PR - please document those as well.

sp.caBytes = block.Bytes
sp.caPrivateKey = key

// generate sever cert / key

This comment has been minimized.

Copy link
@donce

donce Feb 12, 2018

Contributor

server

sp.caBytes = block.Bytes
sp.caPrivateKey = key

// generate sever cert / key

This comment has been minimized.

Copy link
@donce

donce Feb 12, 2018

Contributor

server (mistype)

// generate own ca
ownCA, _ := sp.createCA()

// generate sever cert / key on ownCA

This comment has been minimized.

Copy link
@donce

donce Feb 12, 2018

Contributor

server

@Waldz

Waldz approved these changes Feb 14, 2018

@zolia zolia merged commit f82562b into master Feb 14, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Waldz Waldz deleted the feature/MYST-79-regen-openvpn-keys branch Feb 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.