Skip to content

Commit 8d2e78b

Browse files
enjenj
committed
Correctly handle newlines in SerialFileGenerator
This change makes it so that SerialFileGenerator correctly reads and writes serial files that end in a newline. This allows it to interoperate with other tools that may interact with the serial file such as openssl. Tests were added to assert that the incrementing logic works and that the serial file always ends with a newline. Bug 1512825 Signed-off-by: Monis Khan <[email protected]>
1 parent 7cb6696 commit 8d2e78b

File tree

2 files changed

+78
-21
lines changed

2 files changed

+78
-21
lines changed

‎pkg/cmd/server/crypto/crypto.go

+39-21
Original file line numberDiff line numberDiff line change
@@ -320,27 +320,13 @@ type SerialFileGenerator struct {
320320
Serial int64
321321
}
322322

323-
func NewSerialFileGenerator(serialFile string, createIfNeeded bool) (*SerialFileGenerator, error) {
324-
// read serial file
325-
var serial int64
326-
serialData, err := ioutil.ReadFile(serialFile)
327-
if err == nil {
328-
serial, _ = strconv.ParseInt(string(serialData), 16, 64)
329-
}
330-
if os.IsNotExist(err) && createIfNeeded {
331-
if err := ioutil.WriteFile(serialFile, []byte("00"), 0644); err != nil {
332-
return nil, err
333-
}
334-
serial = 1
335-
336-
} else if err != nil {
323+
func NewSerialFileGenerator(serialFile string) (*SerialFileGenerator, error) {
324+
// read serial file, it must already exist
325+
serial, err := fileToSerial(serialFile)
326+
if err != nil {
337327
return nil, err
338328
}
339329

340-
if serial < 1 {
341-
serial = 1
342-
}
343-
344330
return &SerialFileGenerator{
345331
Serial: serial,
346332
SerialFile: serialFile,
@@ -351,6 +337,16 @@ func NewSerialFileGenerator(serialFile string, createIfNeeded bool) (*SerialFile
351337
func (s *SerialFileGenerator) Next(template *x509.Certificate) (int64, error) {
352338
s.lock.Lock()
353339
defer s.lock.Unlock()
340+
341+
// do a best effort check to make sure concurrent external writes are not occurring to the underlying serial file
342+
serial, err := fileToSerial(s.SerialFile)
343+
if err != nil {
344+
return 0, err
345+
}
346+
if serial != s.Serial {
347+
return 0, fmt.Errorf("serial file %s out of sync ram=%d disk=%d", s.SerialFile, s.Serial, serial)
348+
}
349+
354350
next := s.Serial + 1
355351
s.Serial = next
356352

@@ -359,13 +355,34 @@ func (s *SerialFileGenerator) Next(template *x509.Certificate) (int64, error) {
359355
if len(serialText)%2 == 1 {
360356
serialText = "0" + serialText
361357
}
358+
// always add a newline at the end to have a valid file
359+
serialText += "\n"
362360

363361
if err := ioutil.WriteFile(s.SerialFile, []byte(serialText), os.FileMode(0640)); err != nil {
364362
return 0, err
365363
}
366364
return next, nil
367365
}
368366

367+
func fileToSerial(serialFile string) (int64, error) {
368+
serialData, err := ioutil.ReadFile(serialFile)
369+
if err != nil {
370+
return 0, err
371+
}
372+
373+
// read the file as a single hex number after stripping any whitespace
374+
serial, err := strconv.ParseInt(string(bytes.TrimSpace(serialData)), 16, 64)
375+
if err != nil {
376+
return 0, err
377+
}
378+
379+
if serial < 0 {
380+
return 0, fmt.Errorf("invalid negative serial %d in serial file %s", serial, serialFile)
381+
}
382+
383+
return serial, nil
384+
}
385+
369386
// RandomSerialGenerator returns a serial based on time.Now and the subject
370387
type RandomSerialGenerator struct {
371388
}
@@ -394,7 +411,7 @@ func GetCA(certFile, keyFile, serialFile string) (*CA, error) {
394411

395412
var serialGenerator SerialGenerator
396413
if len(serialFile) > 0 {
397-
serialGenerator, err = NewSerialFileGenerator(serialFile, false)
414+
serialGenerator, err = NewSerialFileGenerator(serialFile)
398415
if err != nil {
399416
return nil, err
400417
}
@@ -431,10 +448,11 @@ func MakeCA(certFile, keyFile, serialFile, name string, expireDays int) (*CA, er
431448

432449
var serialGenerator SerialGenerator
433450
if len(serialFile) > 0 {
434-
if err := ioutil.WriteFile(serialFile, []byte("00"), 0644); err != nil {
451+
// create / overwrite the serial file with a zero padded hex value (ending in a newline to have a valid file)
452+
if err := ioutil.WriteFile(serialFile, []byte("00\n"), 0644); err != nil {
435453
return nil, err
436454
}
437-
serialGenerator, err = NewSerialFileGenerator(serialFile, false)
455+
serialGenerator, err = NewSerialFileGenerator(serialFile)
438456
if err != nil {
439457
return nil, err
440458
}

‎test/cmd/certs.sh

+39
Original file line numberDiff line numberDiff line change
@@ -18,12 +18,29 @@ os::cmd::expect_success_and_not_text \
1818
--overwrite=true" \
1919
'WARNING: .* is greater than 5 years'
2020

21+
os::cmd::expect_success_and_text "cat '${CERT_DIR}/ca.serial.txt'" '00'
22+
os::cmd::expect_success_and_text "tail -c 1 '${CERT_DIR}/ca.serial.txt' | wc -l" '1' # check for newline at end
23+
2124
expected_year="$(TZ=GMT date -d "+$((365*5)) days" +'%Y')"
2225

2326
os::cmd::expect_success_and_text \
2427
"openssl x509 -in '${CERT_DIR}/ca.crt' -enddate -noout | awk '{print \$4}'" \
2528
"${expected_year}"
2629

30+
# Make a cert with the CA to see the counter increment
31+
# We can then check to see if it gets reset due to overwrite
32+
os::cmd::expect_success \
33+
"oc adm create-api-client-config \
34+
--client-dir='${CERT_DIR}' \
35+
--user=some-user \
36+
--certificate-authority='${CERT_DIR}/ca.crt' \
37+
--signer-cert='${CERT_DIR}/ca.crt' \
38+
--signer-key='${CERT_DIR}/ca.key' \
39+
--signer-serial='${CERT_DIR}/ca.serial.txt'"
40+
41+
os::cmd::expect_success_and_text "cat '${CERT_DIR}/ca.serial.txt'" '01'
42+
os::cmd::expect_success_and_text "tail -c 1 '${CERT_DIR}/ca.serial.txt' | wc -l" '1' # check for newline at end
43+
2744
# oc adm ca create-signer-cert should generate certificate with specified number of days and show warning
2845
os::cmd::expect_success_and_text \
2946
"oc adm ca create-signer-cert --cert='${CERT_DIR}/ca.crt' \
@@ -33,6 +50,9 @@ os::cmd::expect_success_and_text \
3350
--expire-days=$((365*6))" \
3451
'WARNING: .* is greater than 5 years'
3552

53+
os::cmd::expect_success_and_text "cat '${CERT_DIR}/ca.serial.txt'" '00'
54+
os::cmd::expect_success_and_text "tail -c 1 '${CERT_DIR}/ca.serial.txt' | wc -l" '1' # check for newline at end
55+
3656
expected_year="$(TZ=GMT date -d "+$((365*6)) days" +'%Y')"
3757

3858
os::cmd::expect_success_and_text \
@@ -58,6 +78,9 @@ os::cmd::expect_success_and_not_text \
5878
--signer-serial='${CERT_DIR}/ca.serial.txt'" \
5979
'WARNING: .* is greater than 2 years'
6080

81+
os::cmd::expect_success_and_text "cat '${CERT_DIR}/ca.serial.txt'" '02'
82+
os::cmd::expect_success_and_text "tail -c 1 '${CERT_DIR}/ca.serial.txt' | wc -l" '1' # check for newline at end
83+
6184
expected_year="$(TZ=GMT date -d "+$((365*2)) days" +'%Y')"
6285
for CERT_FILE in master-client.crt server.crt; do
6386
os::cmd::expect_success_and_text \
@@ -84,6 +107,9 @@ os::cmd::expect_success_and_text \
84107
--expire-days=$((365*3))" \
85108
'WARNING: .* is greater than 2 years'
86109

110+
os::cmd::expect_success_and_text "cat '${CERT_DIR}/ca.serial.txt'" '04'
111+
os::cmd::expect_success_and_text "tail -c 1 '${CERT_DIR}/ca.serial.txt' | wc -l" '1' # check for newline at end
112+
87113
expected_year="$(TZ=GMT date -d "+$((365*3)) days" +'%Y')"
88114

89115
for CERT_FILE in master-client.crt server.crt; do
@@ -106,6 +132,10 @@ os::cmd::expect_success_and_not_text \
106132
--signer-serial='${CERT_DIR}/ca.serial.txt'" \
107133
'WARNING: .* is greater than 2 years'
108134

135+
136+
os::cmd::expect_success_and_text "cat '${CERT_DIR}/ca.serial.txt'" '05'
137+
os::cmd::expect_success_and_text "tail -c 1 '${CERT_DIR}/ca.serial.txt' | wc -l" '1' # check for newline at end
138+
109139
expected_year="$(TZ=GMT date -d "+$((365*2)) days" +'%Y')"
110140
os::cmd::expect_success_and_text \
111141
"openssl x509 -in '${CERT_DIR}/test-user.crt' -enddate -noout | awk '{print \$4}'" \
@@ -125,6 +155,9 @@ os::cmd::expect_success_and_text \
125155
--expire-days=$((365*3))" \
126156
'WARNING: .* is greater than 2 years'
127157

158+
os::cmd::expect_success_and_text "cat '${CERT_DIR}/ca.serial.txt'" '06'
159+
os::cmd::expect_success_and_text "tail -c 1 '${CERT_DIR}/ca.serial.txt' | wc -l" '1' # check for newline at end
160+
128161
expected_year="$(TZ=GMT date -d "+$((365*3)) days" +'%Y')"
129162
os::cmd::expect_success_and_text \
130163
"openssl x509 -in '${CERT_DIR}/test-user.crt' -enddate -noout | awk '{print \$4}'" \
@@ -143,6 +176,9 @@ os::cmd::expect_success_and_not_text \
143176
--key='${CERT_DIR}/example.org.key'" \
144177
'WARNING: .* is greater than 2 years'
145178

179+
os::cmd::expect_success_and_text "cat '${CERT_DIR}/ca.serial.txt'" '07'
180+
os::cmd::expect_success_and_text "tail -c 1 '${CERT_DIR}/ca.serial.txt' | wc -l" '1' # check for newline at end
181+
146182
expected_year="$(TZ=GMT date -d "+$((365*2)) days" +'%Y')"
147183

148184
os::cmd::expect_success_and_text \
@@ -161,6 +197,9 @@ os::cmd::expect_success_and_text \
161197
--expire-days=$((365*3))" \
162198
'WARNING: .* is greater than 2 years'
163199

200+
os::cmd::expect_success_and_text "cat '${CERT_DIR}/ca.serial.txt'" '08'
201+
os::cmd::expect_success_and_text "tail -c 1 '${CERT_DIR}/ca.serial.txt' | wc -l" '1' # check for newline at end
202+
164203
expected_year="$(TZ=GMT date -d "+$((365*3)) days" +'%Y')"
165204

166205
os::cmd::expect_success_and_text \

0 commit comments

Comments
 (0)