add CA Database, switch serial file to text, add CA-Private Key parameter #8

FreshXOpenSource · 2016-09-21T15:05:25Z

The patch allows us to :

generate a CA outside of micromdm
- this allows us to use openssl directly creating the CA
- this allows us to use all the nasty OpenSSL features (such as extensions/crls/oscp etc :)
add the functionality to pass the CA private key password on startup of the server
add the funcitonality to keep track of all issued certificates in the CA Database (see http://pki-tutorial.readthedocs.io/en/latest/cadb.html)
- This allows us to revoke certificates later
switch the serial file format from binary to text

groob

I mentioned a few nits that I'd like addressed,but overall I'm happy to accept this PR.
Thanks for all the work!

Would you be interested in adding a test to ensure that the format of the index.txt file is not mistakenly changed at some point in the future?

groob · 2016-09-21T15:15:59Z

cmd/scepserver/scepserver.go

@@ -46,6 +46,7 @@ func main() {
 		flVersion           = flag.Bool("version", false, "prints version information")
 		flPort              = flag.String("port", envString("SCEP_HTTP_LISTEN_PORT", "8080"), "port to listen on")
 		flDepotPath         = flag.String("depot", envString("SCEP_FILE_DEPOT", "depot"), "path to ca folder")
+		flCAPass            = flag.String("capass", envString("SCEP_CA_PASS", ""), "passwd for the ca.key")


I'm fine with this, but have you considered the case where the key is not a string?

No :-| Lets move this into a TODO. Currently i don't see how to hand this over via command line (unless you use SCEP_CA_PASS? not sure here though)

yeah, we can add a flag if it becomes a necessity :)

groob · 2016-09-21T15:20:44Z

server/depot.go

+	r := bufio.NewReader(file)
+	// is there a better way??
+	data, err := r.ReadString('\r')
+	data = strings.TrimSuffix(data ,"\r")


The bytes package provides equivalent methods for working with bytes.
https://golang.org/pkg/bytes/

See this excellent blog post about using either package https://medium.com/@benbjohnson/go-walkthrough-bytes-strings-packages-499be9f4b5bd#.mpo4ze804

i was just wondering if ReadString('\n') or an equivalent function could not directly remove newlines as supposed but did not find a solution. i guess this is a slightly clumsy but working version we could live with

👍 removing comment

groob · 2016-09-21T15:21:04Z

server/depot.go

+	data, err := r.ReadString('\r')
+	data = strings.TrimSuffix(data ,"\r")
+	data = strings.TrimSuffix(data ,"\n")
+	if err != nil && err != io.EOF {


Can you move the error checking right under the data, err := line?

groob · 2016-09-21T15:23:35Z

server/depot.go

+		return nil, err
+	}
+	serial, success := s.SetString(data,16)
+	if success != true  {


if !success { is clearer
You can also shorten things here with

serial, ok := s.SetString... if !ok { // handle false }

groob · 2016-09-21T15:24:54Z

server/depot.go

+	}
+	serial, success := s.SetString(data,16)
+	if success != true  {
+		return nil, errors.New("Could not convert "+string(data)+" to serial number")


error strings should not be capitalized
https://github.com/golang/go/wiki/CodeReviewComments#error-strings

groob · 2016-09-21T15:27:15Z

server/depot.go

+
+	file, err := os.OpenFile(name, os.O_CREATE | os.O_RDWR | os.O_APPEND, dbPerm)
+	if err != nil {
+	    fmt.Printf("Could not append to "+name+" : %q\n", err.Error())


If you'd like to annotate the error, consider using fmt.Errorf instead.
https://golang.org/pkg/fmt/#Errorf

groob · 2016-09-21T15:28:22Z

server/depot.go

+	dbEntry.WriteString("\n")
+
+	if _, err := file.Write(dbEntry.Bytes()); err != nil {
+		file.Close()


by adding defer file.Close() above you're already ensuring that the file will be closed.

I see that I made the same mistake below :)

👍 was a nasty copy from writeSerial()

FreshXOpenSource · 2016-09-21T16:40:03Z

writing a test for now exceeds my 2 days old GoLang knowledge (and also my currently available time). please accept the PR without this but all issues above fixed.

Meanwhile i added another small feature we need, the validity of a client certificate can now be set via -crtvalid flag in the scep-server. please review and merge if you like it...

groob

Thanks for addressing the changes.

Please set a default duration for validity and I will merge.

groob · 2016-09-21T16:48:01Z

server/depot.go

@@ -146,7 +145,7 @@ func (d fileDepot) writeDB(cn string, serial *big.Int, filename string, cert *x5

 	file, err := os.OpenFile(name, os.O_CREATE | os.O_RDWR | os.O_APPEND, dbPerm)
 	if err != nil {
-		fmt.Printf("Could not append to "+name+" : %q\n", err.Error())
+		fmt.Errorf("could not append to "+name+" : %q\n", err.Error())


notice fmt.Errorf returns an error. so here just change it to
return fmt.Errorf("could not append to "+name+" : %q\n", err.Error())

groob · 2016-09-21T16:48:53Z

cmd/scepserver/scepserver.go

@@ -46,6 +46,7 @@ func main() {
 		flVersion           = flag.Bool("version", false, "prints version information")
 		flPort              = flag.String("port", envString("SCEP_HTTP_LISTEN_PORT", "8080"), "port to listen on")
 		flDepotPath         = flag.String("depot", envString("SCEP_FILE_DEPOT", "depot"), "path to ca folder")
+		flCAPass            = flag.String("capass", envString("SCEP_CA_PASS", ""), "passwd for the ca.key")


yeah, we can add a flag if it becomes a necessity :)

groob · 2016-09-21T16:50:14Z

server/service.go

@@ -162,6 +163,14 @@ func CAKeyPassword(pw []byte) ServiceOption {
 	}
 }

+// optional argument to set the validity of signed client certs in days


The comment should start with // ClientValidity sets the .... The name of the function should come first as per the linter.

the capass flag was necessary because without any password on the ca.key file the enroll process fails... do you know what is the default value in the sign function? maybe "" or sth. strange? or do you know what is the default ca.key password when using scepserver ca -init?

groob · 2016-09-21T16:53:55Z

server/service.go

+// optional argument to set the validity of signed client certs in days
+func ClientValidity(duration int) ServiceOption {
+	return func(s *service) error {
+		s.clientValidity = duration


This is an optional argument, but it defaults to 0 if not set. That does not seem like a good default.

Please change the opts inside NewService so that a default time other than 0 is used. I'll leave it up to you to decide what a good option should be.

isnt
flClDuration = flag.String("crtvalid", envString("SCEP_CERT_VALID", "365"), "validity for new client certificates in days") setting the default?

I see, you're using 365 as a default for the flag. That works

Update README.md and setup CI

Kai Mosebach added 2 commits September 21, 2016 16:55

add CA Database functionality, switch serial file from binary to text

07852df

fix whitespace -> tab

8a32099

groob requested changes Sep 21, 2016

View reviewed changes

groob assigned FreshXOpenSource Sep 21, 2016

Kai Mosebach added 3 commits September 21, 2016 18:12

add -crtvalid flag to set client cert validity, fix date format in CADB

0649c58

Merge branch 'master' of https://github.com/FreshXOpenSource/scep

ba501ed

fix tab/whitespace indent

45a8e6a

Kai Mosebach added 2 commits September 21, 2016 18:40

add cleanups + fixes for merge/PR

6a02d92

add TODO

de9c0a4

groob reviewed Sep 21, 2016

View reviewed changes

get ready for PR merge

5ac77a0

groob approved these changes Sep 21, 2016

View reviewed changes

groob merged commit 59471b1 into micromdm:master Sep 21, 2016

groob mentioned this pull request Sep 21, 2016

create index.txt (the CA DB) in depot folder containing all issued certificates #7

Closed

jessepeterson pushed a commit to jessepeterson/scep that referenced this pull request Sep 25, 2024

Merge pull request micromdm#8 from smallstep/herman/release-prep

a5bad3a

Update README.md and setup CI

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add CA Database, switch serial file to text, add CA-Private Key parameter #8

add CA Database, switch serial file to text, add CA-Private Key parameter #8

FreshXOpenSource commented Sep 21, 2016 •

edited

Loading

groob left a comment

groob Sep 21, 2016

FreshXOpenSource Sep 21, 2016

groob Sep 21, 2016

groob Sep 21, 2016

FreshXOpenSource Sep 21, 2016

FreshXOpenSource Sep 21, 2016

groob Sep 21, 2016 •

edited

Loading

FreshXOpenSource Sep 21, 2016

groob Sep 21, 2016

FreshXOpenSource Sep 21, 2016

groob Sep 21, 2016

FreshXOpenSource Sep 21, 2016

groob Sep 21, 2016

FreshXOpenSource Sep 21, 2016

groob Sep 21, 2016 •

edited

Loading

FreshXOpenSource Sep 21, 2016

FreshXOpenSource commented Sep 21, 2016

groob left a comment

groob Sep 21, 2016

groob Sep 21, 2016

groob Sep 21, 2016

FreshXOpenSource Sep 21, 2016

FreshXOpenSource Sep 21, 2016

groob Sep 21, 2016

FreshXOpenSource Sep 21, 2016

groob Sep 21, 2016

add CA Database, switch serial file to text, add CA-Private Key parameter #8

add CA Database, switch serial file to text, add CA-Private Key parameter #8

Conversation

FreshXOpenSource commented Sep 21, 2016 • edited Loading

groob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groob Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groob Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FreshXOpenSource commented Sep 21, 2016

groob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FreshXOpenSource commented Sep 21, 2016 •

edited

Loading

groob Sep 21, 2016 •

edited

Loading

groob Sep 21, 2016 •

edited

Loading