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 function to use 'HEAD' for 'Certificates' call #12

Closed
wilddev65 opened this issue Jun 14, 2021 · 11 comments · Fixed by #13
Closed

Add function to use 'HEAD' for 'Certificates' call #12

wilddev65 opened this issue Jun 14, 2021 · 11 comments · Fixed by #13
Assignees
Labels
enhancement New feature or request

Comments

@wilddev65
Copy link
Collaborator

Summary of the new feature/enhancement

Currently the Certificates call is used in Find-Certificates which uses the GET method. Additionally available is the 'HEAD' method, which uses the same attributes and status filters as the GET method, but instead returns the number of certificates that matches the filtered criteria in the 'X-Record-Count' header.

This would be useful for situations that simply a count of certificates is needed, for example 'how many certificates were created after 'x' date' or 'how many certificates are currently in error'.

I have attempted to copy the Find-Certificates function into my script and alter it to use HEAD, but I get the error;

Cannot validate argument on parameter 'Method'. The argument "Head" does not belong to the set "Get,Post,Patch,Put,Delete" specified by the
ValidateSet attribute. Supply an argument that is in the set and then try the command again.

@gdbarron
Copy link
Collaborator

This sounds like a simple change, but unfortunately it's not. It's easy enough to add 'Head' to the list of allowable values for 'Method', but as the module is based around Invoke-RestMethod, we don't get the headers. I'll need to update a bit of Invoke-VenafiRestMethod to allow this. I'll also add a switch to Find-TppCertificate to just get the count. As the rest of the params are the same it makes sense to put it there. Sound good?

@gdbarron gdbarron self-assigned this Jun 14, 2021
@gdbarron gdbarron added the enhancement New feature or request label Jun 14, 2021
@wilddev65
Copy link
Collaborator Author

Yeah, I discovered it wasn't simple after just trying to add that verb to Invoke-VenafiRestMethod, as it was still trying to include a body. I saw that Invoke-RestMethod wasn't returning headers when I was developing a Get-TokenRefresh function that was checking the headers in earlier versions of TPP that would only send back '200 Token is Valid' when using 'Authorize/Verify'. I added an elseif that uses Invoke-WebRequest specifically to check for that, lol.
Adding a switch into Find-TppCertificate to handle this use case sounds like the right approach to me.

@gdbarron
Copy link
Collaborator

gdbarron commented Jun 14, 2021

Sounds like you've done some of the work already. Don't hesitate to submit a pull request next time :)

Looks like a Get method returns the count in the header as well as Head. Is there a performance impact in using Get for everything?

@wilddev65
Copy link
Collaborator Author

I keep intending to submit a PR for the Get-TokenRefresh and getting distracted by work. Never done one before, so I'm a bit nervous about it.

The docs say: GET Certificates only returns X.509 certificate values when there is an actual X.509 certificate in Secret Store for the certificate object. If you want the response to return just the total number of matches, use HEAD Certificates instead.
Look from that like GET might not have all the results? I asked about the performance and there is a significant difference between GET vs HEAD, for 10k certs was 30 secs vs 27 ms. Basically it is faster to use HEAD, but at lower numbers of certs it might not be much of a difference.

@gdbarron
Copy link
Collaborator

Thanks for the info. I'll go with 'head' for the count.

As for performing a PR, it's pretty easy and I can help along the way. Here's a good tutorial if you happen to use vscode as I do.

@gdbarron
Copy link
Collaborator

Basics are here, https://github.com/gdbarron/VenafiPS/tree/get-cert-count. The parameter is Find-TppCertificate -CountOnly. I still have some work to do to support 307/401 status codes in Invoke-VenafiRestMethod. If you get time, could you check out what I have so far?

@wilddev65
Copy link
Collaborator Author

Sorry, been really hectic last couple of days and only just able to look at this now.
Looks pretty awesome and your New-HttpQueryString solved an issue I am having right now with trying to build the string for the call. Didn't check out this branch as yet, but will and will run some tests with what I already have got.

@wilddev65
Copy link
Collaborator Author

Okay, ran a quick test and in general things seem to be working. It's assembling the query and a simple HEAD call Find-TppCertificate -CountOnly works to get the total number of certs. It's not assembling the query if you add other params though. I did;
Find-TppCertificate -Path '/VED/Policy/Certificates' -Recursive -CountOnly
and this came back with '0'. I think the issue is that it's still adding the recursive bits into the body and it's not working then as I see from doing Write-Output $params the following;
Name Value


Method Head
Body {Limit, ParentDnRecursive}
UriLeaf certificates/
VenafiSession VenafiSession
FullResponse True

@wilddev65
Copy link
Collaborator Author

Nevermind, that was my typo! I used '/' instead of '' in the path I was feeding it. Once I changed it, this worked fine.

@gdbarron
Copy link
Collaborator

in release v3.1.0

@fearnie85
Copy link

Think this has caused a bug. You can run this but when you add a common name filter. It throws an error. I am on my phone now but will add the error later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants