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

PKI Secrets Engine Role endpoints. #374

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

joevanwanzeeleKF
Copy link

GH-373 PKI Secrets Engine - added methods for interacting with PKI Roles, and method for generating a self-signed root CA.

@joevanwanzeeleKF
Copy link
Author

@konidev20 @rajanadar

@@ -103,7 +103,7 @@ private static VaultClientSettings GetVaultClientSettings(IAuthMethodInfo authMe
Console.WriteLine(_responseContent);
}
},
Namespace = "bhjk",
//Namespace = "bhjk", // ??
Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure why the namespace was in there, but it was causing my runs to fail until I commented it out.

Copy link
Collaborator

Choose a reason for hiding this comment

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

namespaces is a feature available on enterprise versions of Vault. This is a placeholder I reckon. Uncomment before this is merged.

Copy link
Collaborator

@konidev20 konidev20 left a comment

Choose a reason for hiding this comment

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

I am good with the changes.

Just need to look at a couple of things:

  1. How to handle default true booleans for Create/Update Role
  2. Handling of the 404 while listing

@@ -103,7 +103,7 @@ private static VaultClientSettings GetVaultClientSettings(IAuthMethodInfo authMe
Console.WriteLine(_responseContent);
}
},
Namespace = "bhjk",
//Namespace = "bhjk", // ??
Copy link
Collaborator

Choose a reason for hiding this comment

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

namespaces is a feature available on enterprise versions of Vault. This is a placeholder I reckon. Uncomment before this is merged.

@@ -1,3 +1,4 @@

Copy link
Collaborator

Choose a reason for hiding this comment

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

An extra line has been added by mistake.

/// Note: this only has an effect when generating a CA cert or signing a CA cert, not when generating a CSR for an intermediate CA.
/// </summary>
[JsonPropertyName("ttl")]
public string TTL { get; set; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Following the convention used elsewhere in the project. Can we rename this to TimeToLive


namespace VaultSharp.V1.SecretsEngines.PKI
{
public class PKIRole
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion, just Role should be fine.

/// allowing for arbitrary non-Hostname, non-Email address CNs.
/// </summary>
[JsonPropertyName("cn_validations")]
public List<string> CNVAlidations { get; set; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo CNValidations, A has been capitalized

/// ".example.net" or "bz.example.net". See the documentation for more information.
/// </summary>
[JsonPropertyName("allow_wildcard_certificates")]
public bool AllowWildcardIdentifiers { get; set; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

/// If set, an array of allowed user-ids to put in user system login name specified here: https://www.rfc-editor.org/rfc/rfc1274#section-9.3.1
/// </summary>
[JsonPropertyName("allowed_user_ids")]
public List<string> AllowedUserIds { get; set; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a string field according to the docs.

/// If set, an array of allowed serial numbers to put in Subject. These values support globbing.
/// </summary>
[JsonPropertyName("allowed_serial_numbers")]
public List<string> AllowedSerialNumbers { get; set; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a string field. Check and confirm behavior. Should be fine.

/// The maximum allowed lease duration. If not set, defaults to the system maximum lease TTL.
/// </summary>
[JsonPropertyName("max_ttl")]
public long MaxTTL { get; set; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

MaxTimeToLive

Comment on lines +150 to +155
// if the role list is empty, we will get a 404 response with { "errors": [] }
// make sure it's that error only, and return the empty list if so
if (ex.StatusCode != 404)
{
throw;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure about this. A HTTP Status Code 404 can be returned even in the case when the mountPoint is incorrect. Handling the 404 exception might be an application layer decision. I will let @rajanadar comment.

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.

2 participants