Added git ignore file:

[MikhailRezhepp]

This is the first draft of the code. It is only POC (proof-of-concept) and learning exercise and will be cleaned up after the code review.

workflow1() - !!! tests the gRPC endpoints, but the Say..() methods do not do what is expected. I would appreciate help on what is incorrect. workflow2() - reads from the vault and un-marshals the results workflow3() - tests List() and Read() methods on the Vault, also creates a certificate and reads it for verification.

---

This change is 

[bfrancojr]

Review status: 0 of 5 files reviewed at latest revision, 7 unresolved discussions.

---

trustedentity/client/main.go, line 197 at r2 (raw file):

// test_workflow4() - creates a certificate and reads it for verification.
//
func test_workflow1() {

please move all those test_* functions to the main_test.go file take a look: https://medium.com/@thejasbabu/testing-in-golang-c378b351002d

---

trustedentity/server/main.go, line 37 at r2 (raw file):

const (
  port = ":50051"
  vaultAddr = "http://192.168.0.50:8200"

this address should be configurable, otherwise it is not going to work in any other machine.

---

trustedentity/server/main.go, line 47 at r2 (raw file):

  privateKey interface {}
  privateKeyType interface {}
  serialNumber interface {}

those are all strings fields. why are you using interface{} here?

---

trustedentity/server/main.go, line 51 at r2 (raw file):

// Globally declared vault variable
var vault *vaultapi.Logical = initVault()

Let's avoid the use of global vars. Take a look at this presentation: https://talks.golang.org/2013/bestpractices.slide#1 It may help to get your code closer to some best golang practices.

---

trustedentity/server/main.go, line 148 at r2 (raw file):

          "ttl":         ttl,
      })
  if err == nil {

try to simplify the error handling branching a pattern like:

if err != nil { return err } ...

makes the code cleaner by avoid more complex structures like ifs and elses.

---

trustedentity/server/main.go, line 149 at r2 (raw file):

      })
  if err == nil {
      certificate = strings.TrimSpace(s1.Data["certificate"].(string))

you may consider initialize PKIDataType structure in one pass. something like: return &PKIDataType{ certificate: strings.TrimSpace(s1.Data["certificate"].(string)), .... }

---

trustedentity/server/main.go, line 184 at r2 (raw file):

func (s *secretKeeperServer) SayCACertificate(ctx context.Context, in *pb.CACertificateRequest) (*pb.CACertificateReply, error) {
  //log.Printf("Server Step SayCACertificate v" + serverVersion)
  const caCertPath = "pki/cert/ca"

if a const var is used inside of this function only and just once, I don't see much sense to declare them. please considerer review all other functions regarding this suggestion.

---

Comments from Reviewable

[MikhailRezhepp]

trustedentity/server/main.go, line 184 at r2 (raw file):

Previously, bfrancojr (Bene) wrote…

if a const var is used inside of this function only and just once, I don't see much sense to declare them. please considerer review all other functions regarding this suggestion.

In my view this coding style makes the code more readable. A reader will clearly see that the second parameter is "itemTitle". Are declarations expensive? The compiler will do a similar job anyway. E.g. a call to MyFunc("xyz") will be compiled as: ... const $tempVar1 = "xyz" MyFunc($tempVar1)

So my goal was readability. I could surely add comments to clarify that the second parameter is "item title", but the named variable serves the same purpose without unnecessary cluttering the code.

---

Comments from Reviewable

[MikhailRezhepp]

trustedentity/server/main.go, line 184 at r2 (raw file):

Previously, MikhailRezhepp wrote…

In my view this coding style makes the code more readable. A reader will clearly see that the second parameter is "itemTitle". Are declarations expensive? The compiler will do a similar job anyway. E.g. a call to MyFunc("xyz") will be compiled as: ... const $tempVar1 = "xyz" MyFunc($tempVar1) So my goal was readability. I could surely add comments to clarify that the second parameter is "item title", but the named variable serves the same purpose without unnecessary cluttering the code.

It is also better suited for potential changes. If the same parameter "itemTitle" will be used in a call to MyFunc2, I can reference the variable. Later if item title changes, it could be done in one place, rather than searching for all occurrences of "CA certificate" and replacing them with a new value.

---

Comments from Reviewable

[MikhailRezhepp]

trustedentity/server/main.go, line 47 at r2 (raw file):

Previously, bfrancojr (Bene) wrote…

those are all strings fields. why are you using interface{} here?

Fixed this

---

Comments from Reviewable

[MikhailRezhepp]

trustedentity/server/main.go, line 148 at r2 (raw file):

Previously, bfrancojr (Bene) wrote…

try to simplify the error handling branching a pattern like: if err != nil { return err } ... makes the code cleaner by avoid more complex structures like ifs and elses.

Will this be a right way to handle errors?

if err != nil {
    log.Printf("Error in createPKICertificate: %s", err)
    return PKIDataType {}, err
    }
return PKIDataType {
    certificate: strings.TrimSpace(s1.Data["certificate"].(string)),
    issuingCA: strings.TrimSpace(s1.Data["issuing_ca"].(string)),
    privateKey: strings.TrimSpace(s1.Data["private_key"].(string)),
    privateKeyType: s1.Data["private_key_type"].(string),
    serialNumber: s1.Data["serial_number"].(string),
}, nil

---

Comments from Reviewable

[MikhailRezhepp]

trustedentity/server/main.go, line 148 at r2 (raw file):

Previously, MikhailRezhepp wrote…

Will this be a right way to handle errors? if err != nil { log.Printf("Error in createPKICertificate: %s", err) return PKIDataType {}, err } return PKIDataType { certificate: strings.TrimSpace(s1.Data["certificate"].(string)), issuingCA: strings.TrimSpace(s1.Data["issuing_ca"].(string)), privateKey: strings.TrimSpace(s1.Data["private_key"].(string)), privateKeyType: s1.Data["private_key_type"].(string), serialNumber: s1.Data["serial_number"].(string), }, nil

By the way, why GO is so negative to "if ... then ... else ..." programming pattern? I personally find it clear and concise and better modifiable, rather than falling through the flow. Is this all about saving CPU cycles?

---

Comments from Reviewable

[MikhailRezhepp]

trustedentity/server/main.go, line 149 at r2 (raw file):

Previously, bfrancojr (Bene) wrote…

you may consider initialize PKIDataType structure in one pass. something like: return &PKIDataType{ certificate: strings.TrimSpace(s1.Data["certificate"].(string)), .... }

Done.

---

Comments from Reviewable

[MikhailRezhepp]

trustedentity/server/main.go, line 51 at r2 (raw file):

Previously, bfrancojr (Bene) wrote…

Let's avoid the use of global vars. Take a look at this presentation: https://talks.golang.org/2013/bestpractices.slide#1 It may help to get your code closer to some best golang practices.

Removed the global variable

---

Comments from Reviewable

[MikhailRezhepp]

Review status: 0 of 9 files reviewed at latest revision, 8 unresolved discussions.

---

trustedentity/client/main.go, line 161 at r2 (raw file):

certificate = strings.TrimSpace(s1.Data["certificate"].(string)) issuingCA = strings.TrimSpace(s1.Data["issuing_ca"].(string)) privateKey = strings.TrimSpace(s1.Data["private_key"].(string)) privateKeyType = s1.Data["private_key_type"].(string) serialNumber = s1.Data["serial_number"] //log.Printf("createPKICertificate.certificate: %s", certificate) //log.Printf("createPKICertificate.issuing_ca: %s", issuingCA) //log.Printf("createPKICertificate.private_key: %s ( key type: %s)", privateKey, privateKeyType) //log.Printf("createPKICertificate.serial_number: %v", serialNumber)

---

trustedentity/client/main.go, line 197 at r2 (raw file):

Previously, bfrancojr (Bene) wrote…

please move all those test_* functions to the main_test.go file take a look: https://medium.com/@thejasbabu/testing-in-golang-c378b351002d

Done.

---

trustedentity/server/main.go, line 37 at r2 (raw file):

Previously, bfrancojr (Bene) wrote…

this address should be configurable, otherwise it is not going to work in any other machine.

Done.

---

Comments from Reviewable

[bfrancojr]

Reviewed 1 of 5 files at r1, 8 of 8 files at r3. Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[bfrancojr]

Review status: 8 of 9 files reviewed at latest revision, 1 unresolved discussion.

---

trustedentity/client/main.go, line 161 at r2 (raw file):

Previously, MikhailRezhepp wrote…

> certificate = strings.TrimSpace(s1.Data["certificate"].(string)) > issuingCA = strings.TrimSpace(s1.Data["issuing_ca"].(string)) > privateKey = strings.TrimSpace(s1.Data["private_key"].(string)) > privateKeyType = s1.Data["private_key_type"].(string) > serialNumber = s1.Data["serial_number"] > //log.Printf("createPKICertificate.certificate: %s", certificate) > //log.Printf("createPKICertificate.issuing_ca: %s", issuingCA) > //log.Printf("createPKICertificate.private_key: %s ( key type: %s)", privateKey, privateKeyType) > //log.Printf("createPKICertificate.serial_number: %v", serialNumber)

ok

---

Comments from Reviewable

[bfrancojr]

Reviewed 1 of 1 files at r4. Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable