gdbarron / VenafiTppPS

Deprecated, see VenafiPS project. PowerShell module to access the features of Venafi Trust Protection Platform REST API
MIT License
18 stars 5 forks source link

module version 0.7.3 fails to connect to Venafi server 18.4 #68

Closed tristanbarcelon closed 5 years ago

tristanbarcelon commented 5 years ago

Hi @gdbarron , Our security admins recently updated our Venafi server to 18.4. I recall that you had made changes for 18.3 in 0.7.3 but this version is unable to connect. Switching back to 0.7.2 works. Likewise, I'm also able to invoke Get-TppCertificate successfully on 0.7.2. I get an error like this on 0.7.3 with the Verbose switch on. I will dig deeper next week when I have available time. I suspect the issue the error has to do with changes to TppSession class and the switch to SystemStatus instead of SystemStatus/Version.

Invoke-RestMethod : {"Error":"Internal error occurred."}
At C:\Program Files\WindowsPowerShell\Modules\venafitppps\0.7.3\Private\Invoke-TppRestMethod.ps1:105 char:9
+         Invoke-RestMethod @params
+         ~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : InvalidOperation: (System.Net.HttpWebRequest:HttpWebRequest) [Invoke-RestMethod], WebException
    + FullyQualifiedErrorId : WebCmdletWebResponseException,Microsoft.PowerShell.Commands.InvokeRestMethodCommand
gdbarron commented 5 years ago

Hi @tristanbarcelon. Powershell has a bug where classes can not be reloaded on the fly and any attempt to do so will result in failure when you try to use them. I see the same issue when I have worked with a prior release and then try and import the latest version which has class updates; something is bound to fail. You need to open a new PS session and load the version you want to use straightaway. Can you see if this solves your issue?

using module was meant to be a bit more "class friendly", but looks to not reload them either.

I don't think this has to do with the difference between SystemStatus and SystemStatus/Version because I didn't have calls to either in the prior version. I'm using SystemStatus to determine the system version as opposed to SystemStatus/Version as SystemStatus has been around for quite some time and /Version was just introduced in 18.3 Both are available in 18.4 so that shouldn't be an issue.

tristanbarcelon commented 5 years ago

Hi @gdbarron , thanks for your feedback. I went back and started a new session just to confirm. In this new session, only 0.7.3 is loaded but both 0.7.2 and 0.7.3 are installed.

image

I pulled up fiddler to see what was happening. Could the last call to SystemStatus be the culprit? It was redirected to SystemStatus/ and that's what threw the error.

image

image

On a separate session of powershell and using version 0.7.2, there is no extra call to SystemStatus as shown here.

image

gdbarron commented 5 years ago

I think SystemStatus might be the culprit as well. Can you load up 0.7.2, create a new session and then execute Invoke-WebRequest -Uri ('{0}/vedsdk/SystemStatus' -f $TppSession.ServerUrl) -Method Get -Headers @{"X-Venafi-Api-Key" = $TppSession.ApiKey} and let me know what happens? Then, try with 0.7.3. The one time I got the error you reported, the session was created, it just didn't have the version property populated. You can see the current session, if there is one, with $TppSession. Thanks. image

tristanbarcelon commented 5 years ago

Hi @gdbarron , I get the following error which is very similar to 0.7.3.

image

using SystemStatus/Version does work as shown in this sequence.

image

image

gdbarron commented 5 years ago

Not sure about you, but this looks like an api bug to me. If you agree, I can back out the new code. Would you be able to open a bug with Venafi?

tristanbarcelon commented 5 years ago

It's one of the two things: incorrect sdk doc or bug in the api. The link you used in commit bedba65 for Get-TppSystemStatus.ps1 references 17.4

https://docs.venafi.com/Docs/17.4SDK/TopNav/Content/SDK/WebSDK/API_Reference/r-SDK-GET-SystemStatus.php?tocpath=REST%20API%20reference%7CServiceStatus%20programming%20interfaces%7C_____1

I'm logging in now to venafi to see if I can find the service status api. I wish they hadn't required a login just to view the SDK documentation.

Update: Just logged in and changed the docs url to the one below and the signature looks the same as 17.4

https://docs.venafi.com/Docs/18.4SDK/TopNav/Content/SDK/WebSDK/API_Reference/r-SDK-GET-SystemStatus.php?tocpath=REST%20API%20reference%7CServiceStatus%20programming%20interfaces%7C_____1

Looking at their docs, version field should be available from SystemStatus even without using SystemStatus/Version so they definitely broke the SystemStatus api. Unfortunately, we no longer have a 17.x system to test against.

image

Update: we are now in communication with Venafi support and I'll update this ticket once we know more.

gdbarron commented 5 years ago

Thanks for confirming. The links in my help do not account for possible changes to the method parameters over time. Right now the assumption is either a method exist or doesn't. I understand that's not the reality of the API provided, but to account for that would be a pretty large undertaking and not one I can currently take on.

The framework I added in this release was to account for new methods as a whole. As the Version API was added in 18.3, I figured using SystemStatus which has been around for quite some time would be a better bet. I will roll that back for now.

tristanbarcelon commented 5 years ago

That's cool. At the very least, I was able to navigate through the markdown and find the SystemStatus link. I can't even say that for the module I maintain for internal use. Will need to go through your repo and CI process more to figure out how the markdown files and manifest get updated automatically. Right now, I must manually update the manifest to account for updated list of public functions and it is very error prone. The only thing I've automated is the versioning (by tag), manifest/nuspec version (from tag), and script signing. Still missing pester and psscriptanalyzer tests in CI as I do that one manually before committing in Git.

gdbarron commented 5 years ago

69. Please let me know if you are able to open a ticket with Venafi and hear anything back. Thanks much.

gdbarron commented 5 years ago

Hi @tristanbarcelon. Any chance you were able to connect with Venafi on this?

BeardedPrincess commented 5 years ago

Hey Greg.. what is the issue on this? I can open a bug easily enough.. but reading through this I'm not sure i completely understand what's going on.

-Justin

On Mon, Feb 25, 2019, 09:32 Greg Brownstein notifications@github.com wrote:

Hi @tristanbarcelon https://github.com/tristanbarcelon. Any chance you were able to connect with Venafi on this?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/gdbarron/VenafiTppPS/issues/68#issuecomment-467032720, or mute the thread https://github.com/notifications/unsubscribe-auth/AbBjZhdo9L78vsf3quE4v0KuoHaj3GWfks5vQ_QKgaJpZM4a8cg6 .

gdbarron commented 5 years ago

Hi Justin. It looks as if we're seeing intermittent issues with /SystemStatus in 18.4. I am able to repro this on 1 system with 18.4 and not another. I believe @tristanbarcelon can repro this as well.

We are getting "Internal error occurred" as you can see in the screenshots above. At first I thought the issue was related to loading classes in PowerShell, but we are seeing failures with a simple invoke-webmethod api call. Let us know if there's anything we can provide to move this forward.

@tristanbarcelon did mention he has a ticket open. Just FYI to possibly avoid duplicate efforts.

tristanbarcelon commented 5 years ago

Hi @gdbarron . Sorry for the late reply as I was out all week last week. We were given this ticket link 45279 but unfortunately, I'm not able to log into the support portal. It appears that only our security team can log in. @gdbarron Since you are able to repro the error in one 18.4 system and not the other, I am wondering whether there are certain permissions required to call /SystemStatus api and it is assigned in one but not the other. I do know that if I access certificate paths that we don't own, we will get an error when attempting to download or renew cert.

BeardedPrincess commented 5 years ago

Hey guys, I'm on the road this week, but will look into this. If it's a bug i can get it reported and keep track of it for us. Remind me next week if you haven't heard back from me by then.

Greg: we should bring up with Ben getting you a support account so you can report issues too.. although I'm always happy to help with that!

On Tue, Feb 26, 2019, 23:55 tristanbarcelon notifications@github.com wrote:

Hi @gdbarron https://github.com/gdbarron . Sorry for the late reply as I was out all week last week. We were given this ticket link https://support.venafi.com/hc/requests/45279 but unfortunately, I'm not able to log into this portal. Only our security team can log in it appears.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/gdbarron/VenafiTppPS/issues/68#issuecomment-467724160, or mute the thread https://github.com/notifications/unsubscribe-auth/AbBjZk4wB84KCGuzAIPhUQKgMlbQH7XAks5vRg_QgaJpZM4a8cg6 .

gdbarron commented 5 years ago

Just saw this in the docs, "The caller must have Read permission to the engine root.". This is for /SystemStatus whereas /SystemStatus/Version requires no additional permissions. Justin, where in the gui can I confirm I have this permission?

BeardedPrincess commented 5 years ago

In WebAdmin.. if you navigate to the "Platforms" tree, then right click the root node and goto permissions. From there you can grant 'View' and 'Read' to those who need access. The feedback I gave the product team was that getting the version of the current platform should not require additional permissions.. so that's how I'd expect the /SystemStatus/Version to behave. Is that not the case?

If you're using a 'master admin' account, it has everything access by default (and won't be listed there). You can goto the Identities tree and lookup your user account to see if it has master admin rights.

gdbarron commented 5 years ago

That's looks to be the case and what tristanbarcelon and I are seeing are most likely due to permissions. Do you have a sense as to how well adopted 18.3, where /SystemStatus/Version was added, is? I don't want to make framework level updates that require functionality most people don't have. Given we're only at 18.4, I suspect I'll wait to add this back in, but leave Get-TppSystemStatus in place with a warning unless you think that's being over cautious.

BeardedPrincess commented 5 years ago

The reason getting the version is important to this framework to begin with, is so that you can take advantages of improvements to the API as they are released. For example, a new /Certificates/Associate function was introduced in 18.2 which greatly simplified the minimum of 3 /config/write calls necessary to associate a certificate to an application. Then, in 18.3 that same function got a new parameter that allows customers to not only associate a certificate to an app, but to initiate the installation that certificate by setting PushToNew = true. (The default is false, to maintain original functionality).

The way I'd expect this to be used for functions in here would be to check if the version running supports some new thing, and if so use the new (often simpler) way. If it is not supported, use the old approach. (Then, as those old TPP versions go out of support, you can remove the older code)

For this reason, I'd still perform a /SystemStatus/Version on every system, but wrap it in a try/catch. If you get a 404, you'll know that the version is less than 18.3. That could be sufficient moving forward to add support for newly introduced functions, since they'll never be released for 17.4 for example. It's not perfect, but at least it helps moving forward. We'll continue to support 18.2 until 20.2 releases next year.

gdbarron commented 5 years ago

Thanks Justin. I agree with using the version to know when certain functions/features are or aren't available, but need to give some more thought to the simplification side of things. I wouldn't want to write code twice just to simplify when it's already been done. But, if the code that's already been written may no longer work, that's another story. Also, it going out of support doesn't necessarily mean people aren't still running it and I wouldn't want to lock them out either.

I do like your idea of being able to tell if we're running before or after/on 18.3. It's not perfect, as you say, but it's something 😃

18.4 docs had a great run down of the API changes, but not as much with prior versions. If we could keep that going, it would be a big help in maintaining this.

tristanbarcelon commented 5 years ago

Hi @BeardedPrincess @gdbarron our admin set the perms as discussed to the automation account. image I then tried to access myserver03 directly using version 0.7.3 which calls into SystemStatus but still get the same error. Shouldn't that have caused a 401 instead of an internal server error 5xx if it's indeed a perm issue? The account we use is not a master admin, if that helps. When we made it a master admin account, the call to SystemStatus did work.

Update Had our admin grant view/read permissions at engine root and it now works without being a master admin based on this rest api doc.

@gdbarron Would you prefer a code change that traps a 5xx status when calling SystemStatus and throwing a friendlier error message e.g. "You need read/view permissions at engine root level to call SystemStatus"?

BeardedPrincess commented 5 years ago

@tristanbarcelon It looks like you were granted permissions only at that individual server level. In order for GET /systemstatus to work, you need View/Read at the 'root' of the Platforms Tree, like this:

image

tristanbarcelon commented 5 years ago

@BeardedPrincess , setting it at this level would negate the need to apply it on each node? We happen to have 6 servers right now.

BeardedPrincess commented 5 years ago

Correct.

I'm also not entirely sure that setting it at each level would even work. There may be attributes that are being read on the root node to present the /SystemStatus information. (Plus, if more servers are added later, they will inherit that permission from the root as well)

tristanbarcelon commented 5 years ago

Thank you for the clarification @BeardedPrincess