Skip to content

deps: update github.com/cloudflare/cfssl to v1.3.4.#4374

Closed
cpu wants to merge 1 commit intomasterfrom
cpu-cfssl-bump-190730
Closed

deps: update github.com/cloudflare/cfssl to v1.3.4.#4374
cpu wants to merge 1 commit intomasterfrom
cpu-cfssl-bump-190730

Conversation

@cpu
Copy link
Copy Markdown
Contributor

@cpu cpu commented Jul 30, 2019

This will unblock pre-issuance linting support by updating the github.com/cloudflare/cfssl dependency to the 1.3.4 tag which notably includes the zlint integration developed in cloudflare/cfssl#1015

Updates #4255

deps: update cfssl to v1.3.4.
8efe7f7 
This will unblock pre-issuance linting support.
@cpu cpu requested a review from a team as a code owner July 30, 2019 19:19
@cpu cpu self-assigned this Jul 30, 2019
@cpu
Copy link
Copy Markdown
Contributor Author

cpu commented Jul 30, 2019

Looks like some real test failures to sort out 🤔

rolandshoemaker
rolandshoemaker suggested changes Jul 30, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@rolandshoemaker rolandshoemaker left a comment

Choose a reason for hiding this comment

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

Oh boy, uh, tests look unhappy.

@cpu
Copy link
Copy Markdown
Contributor Author

cpu commented Jul 30, 2019

I'm not sure why yet, but it looks like the CAConfig in the failing tests are marshaling a cfsslConfig.Config that has a string value for what's defined as a zlint.lints.LintStatus (e.g. int) field:

"lint_error_level": "",

That's breaking with an error like:

ca_test.go:921: Failed to create CA: {"code":5200,"message":"failed to unmarshal configuration: json: cannot unmarshal string into Go struct field SigningProfile.lint_error_level of type lints.LintStatus"}

@cpu
Copy link
Copy Markdown
Contributor Author

cpu commented Jul 30, 2019

So far I'm pretty baffled. I traced the error to here:

boulder/ca/ca.go

Lines 209 to 214 in bb005e1

// CFSSL requires processing JSON configs through its own LoadConfig, so we
// serialize and then deserialize.
cfsslJSON, err := json.Marshal(config.CFSSL)
if err != nil {
return nil, err
}

Prior to that line the config.CFSSL.Signing.Profiles["ecdsaEE"].LintErrLevel and config.CFSSL.Signing.Profiles["rsaEE"].LintErrLevel) are both int(0) as expected for a default value of a zlint.lint.LintStatus field.

After that line the JSON produced has lint_err_level values like: "lint_error_level":"". What is causing the json package to marshal a zlint.lints.LintStatus field defined as type LintStatus int as "" for the default value ❓ ❓ ❓ ❓

@cpu
Copy link
Copy Markdown
Contributor Author

cpu commented Jul 30, 2019

Closing this PR for now. @rolandshoemaker spotted the bug (:tada:) and it will need to be addressed in the upstream zlint or cfssl project before I can continue here.

@cpu cpu closed this Jul 30, 2019
@cpu
Copy link
Copy Markdown
Contributor Author

cpu commented Jul 30, 2019

Put up a fix PR with the zlint upstream: zmap/zlint#297 This branch will be blocked on it being merged and the Boulder zlint dep being updated (see #4375)

@cpu cpu mentioned this pull request Jul 31, 2019
Merged
@cpu cpu deleted the cpu-cfssl-bump-190730 branch November 8, 2019 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jsha jsha jsha approved these changes

+1 more reviewer

@rolandshoemaker rolandshoemaker rolandshoemaker requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@cpu cpu

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cpu @jsha @rolandshoemaker