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

Add PKCS8 marshaling to PKI #3518

Merged
merged 4 commits into from
Nov 6, 2017
Merged

Add PKCS8 marshaling to PKI #3518

merged 4 commits into from
Nov 6, 2017

Conversation

jefferai
Copy link
Member

@jefferai jefferai commented Nov 2, 2017

No description provided.

@jefferai jefferai added this to the 0.8.4 milestone Nov 2, 2017
@@ -22,6 +22,17 @@ key and issuing cert will be appended to the
certificate pem. Defaults to "pem".`,
}

fields["private_key_format"] = &framework.FieldSchema{
Copy link
Member

Choose a reason for hiding this comment

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

Add this param to the docs page as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@@ -106,6 +106,13 @@ func (b *backend) pathGenerateIntermediate(
}
}

if data.Get("private_key_format").(string) == "pkcs8" {
Copy link
Member

Choose a reason for hiding this comment

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

This is fine as-is, but noticed that the other params were being retrieved with b.getGenerationParams(data), so this could also be moved there as well

Copy link
Member Author

Choose a reason for hiding this comment

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

I went down that path originally but there lay madness. The problem is that it's not called by all functions that can generate certs and what is then done with the generation params depends on the path.

After going down that path and then one other, I realized the easiest way by far was to just transform the output once it's already generated.

Copy link
Member

@vishalnayak vishalnayak left a comment

Choose a reason for hiding this comment

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

One minor comment. Otherwise, LGTM!


privKeyTypeRaw, ok := resp.Data["private_key_type"]
if !ok {
return fmt.Errorf("error converting response to pkcs8: %q not found in response", "private_key_type")
Copy link
Member

Choose a reason for hiding this comment

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

Can we not use %q here since its not taking any variable as input? Or was there a reason for doing it this way?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did it this way instead of \". Just felt nicer than escapes.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. 👍

@jefferai jefferai merged commit 33cf980 into master Nov 6, 2017
@jefferai jefferai deleted the marshal-pkcs8 branch November 6, 2017 17:05
chrishoffman pushed a commit that referenced this pull request Nov 7, 2017
* oss/master: (30 commits)
  Handle 'not supplied' case for field type TypeNameString (#3546)
  Fix deprecated cassandra backend tests (#3543)
  changelog++
  auth/aws: Make disallow_reauthentication and allow_instance_migration mutually exclusive (#3291)
  changelog++
  More Mount Conflict Detection (#2919)
  Fix swallowed errors in TestRollbackManager_Join() (#3327)
  changelog++
  added AWS enpoint handling (#3416)
  Seal wrap all root tokens and their leases (#3540)
  Return group memberships of entity during read (#3526)
  Add note on support for using rec keys on /sys/rekey (#3517)
  Add third party tools list to website (#3488)
  Minor client refactoring (#3539)
  changelog++
  Add PKCS8 marshaling to PKI (#3518)
  Update SSH list roles docs (#3536)
  Update gocql dep
  changelog++
  Return role info for each role on pathRoleList (#3532)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants