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

Improve "Git Commit Information" reference documentation #24205

Closed
xenoterracide opened this issue Nov 18, 2020 · 9 comments
Closed

Improve "Git Commit Information" reference documentation #24205

xenoterracide opened this issue Nov 18, 2020 · 9 comments

Comments

@xenoterracide
Copy link
Contributor

@xenoterracide xenoterracide commented Nov 18, 2020

So... I could potentially report this as a security vulnerability, but it's already easily mitigated, and pretty minor. I suspect this will get closed. This issue may also be better remedied in the BuildInfo plugins for maven/gradle.

/actuator/info leaks information that allows an attacker to determine what vulnerabilities may be available. Specifically datetime's. If I know the build/last git time I know what libraries were available, and can even guess at the version of the JVM.

my preferred solution would be for info to make like health and not show all of the data unless you're authenticated.

Another solution is to null out time for both build and git, currently this is possible with build in the gradle plugin using kotlin, but not possible with git as there is no api exposed to set that property.

lastly of course, you can just use these properties to make them require authentication.

management.endpoints.web.exposure.exclude  
management.endpoints.web.exposure.include info, health

and secure them further https://docs.spring.io/spring-boot/docs/current/reference/htmlsingle/#production-ready-endpoints-security

just an idea.

@mbhave
Copy link
Contributor

@mbhave mbhave commented Nov 18, 2020

IIRC, by default the info endpoint returns an empty json unless an explicit InfoContributor bean is provided or one the auto-configured info contributors is enabled. If an application does so explicitly, it is upto the application developer to evaluate whether that information is sensitive and if so secure the info endpoint. Since the default is secure, in my opinion we shouldn't do anything. Let's see what the rest of the team thinks.

@philwebb
Copy link
Member

@philwebb philwebb commented Nov 18, 2020

I quite like the idea of hiding the time values from the GitInfoContributor.

@mbhave
Copy link
Contributor

@mbhave mbhave commented Nov 18, 2020

I don't think I see a use case for having an unauthenticated response and an authenticated response for the info endpoint. For the health endpoint, there can be a monitoring system which might just need to get the status and then an authenticated user can get more information. Since the info endpoint doesn't include any information by default, I don't see the benefit of hiding that information when enabled.

@bclozel
Copy link
Member

@bclozel bclozel commented Nov 19, 2020

Looking at the /info response structure, nulling out time might not be enough anyway: the commit id and app version also provide the same information. If they've not changed in a while, the application hasn't been upgraded.

I think the current arrangement strikes a nice balance and I'm in favor of keeping things that way.

@wilkinsona
Copy link
Member

@wilkinsona wilkinsona commented Nov 19, 2020

Our Maven plugin for generating the build info can be configured not to include the time and the Gradle plugin can too.

Similar capabilities are also available in the Maven and Gradle plugins for generating the git properties. The Maven plugin allows the properties that are included to be configured and the Gradle plugin does too:

gitProperties {
    keys = ['git.branch','git.commit.id','git.commit.time']
}

One complication here is our management.info.git.mode property. It restricts the git properties that are exposed to
git.branch, git.commit.id, and git.commit.time unless it's set to full. We could move git.commit.time behind the full mode but, given the more flexible configuration provided by the plugins, I wonder if we need this property at all now.

@philwebb
Copy link
Member

@philwebb philwebb commented Nov 19, 2020

One advantage of keeping the property is it's possible to switch things at runtime. E.g. you can set the plugin to include everything and then later decide if you show it or not.

@jeffbswope
Copy link

@jeffbswope jeffbswope commented Dec 4, 2020

Not to advocate for spring boot solving this problem, but to potentially present a real use case:

Internally we have a tool that wants to collect and validate installed versions across dozens of applications/installations. This is a antiquated tool for which providing all of the actuator credentials would be inappropriate/onerous, and it only really needs the version/artifact information.

We also use or have developed many InfoContributors that provide a lot of extra information about the build/source/application/environment/integration-points which is potentially sensitive. Many places we often use Spring Boot Admin, where this /info can be securely fetched and presented in a useful way out-of-the-box. And authorized consumers who want/need that additional, potentially-sensitive information could be provided the appropriate credentials.

Our current plan is to create a /version endpoint (where we might expose the non-sensitive information from selected InfoContributors) for the old version checker to do its limited job and secure the /info endpoint so we can (continue to) expose potentially-sensitive info there -- which is probably the best solution in any case.

But what we are trying to achieve is sort of the same use case as the /health endpoint for unauthenticated vs authenticated users, albeit with potentially more complex decisions/logic about what's public vs not. In fact historically many things which might have been exposed as a SensitiveInfoContributor -- or have different details for auth/no-auth -- have instead been relegated to health indicator details in order to borrow that security, with the small price of being tied to the larger health check(s).

@philwebb
Copy link
Member

@philwebb philwebb commented Dec 16, 2020

We had quite a long discussion about this issue today to think about the options. We ultimately decided that we're going to leave things as they are but update the "Git Commit Information" section as it's a bit lacking.

Here's a summary of some of the ideas that we discussed:

Don't show all of the data unless you're authenticated

There would be a bit of work for us to do to support this, but ultimately the problem becomes about categorizing the information to decide when it should be shown. For example, should we include the git sha, but exclude the timestamp by default? How do users change our defaults? How do users define which of their own entries get shown or hidden?

It's certainly would require a fair amount of design work.

Introduce something like health groups

We could introduce some kind of info group solution where you can assign info entries to groups. This might allow you to categorize sensitive vs insensitive entries. To be of use, we'd probably also need to support authentication so that we can have a property similar to management.endpoint.health.group.custom.roles.

Update the GitInfoContributor.Mode enum

We could update GitInfoContributor.Mode to add a Mode.LIMITED which would hide the git time and make this the default. For a couple of reasons, we weren't sure we should do this:

  1. The Maven and Gradle plugins already provide a way to configure the properties that are written to the file.

  2. Hiding the git date isn't really enough. A competent hacker can monitor the git sha to tell when an application gets updated. All they need to do is hit the /info endpoint each day and see if the SHA has changed.

Don't expose info by default

We discussed disabling the /info endpoint by default and forcing users to opt-in to it. We ultimately decided that since the endpoint only exposes info that you configure, it seemed that our current defaults aren't too big a problem.


There's quite a few idea here that we might revisit if we get time. I'm personally quite keen on the info groups idea, but it will take quite a bit of design work and I'm not sure it's a high enough priority at the moment.

@philwebb philwebb changed the title Secure sensitive /actuator/info information Imrpove "Git Commit Information" reference documentation Dec 16, 2020
@philwebb philwebb added this to the 2.3.x milestone Dec 16, 2020
@xenoterracide
Copy link
Contributor Author

@xenoterracide xenoterracide commented Dec 16, 2020

Perhaps /info should become an authenticated endpoint by default like everything but /health (in the next minor version)? also, I believe buildInfo in the gradle plugin adds a date (could be wrong and not looking at it at it this second, but that documentation doesn't suggest how to squelch it, and I believe I looked for a property with kotlin DSL, and I didn't see one. I could be completely wrong about all of that.

In any sense, thank you for the explanation

@mbhave mbhave changed the title Imrpove "Git Commit Information" reference documentation Improve "Git Commit Information" reference documentation Dec 16, 2020
@mbhave mbhave closed this in 302ba77 Dec 19, 2020
@mbhave mbhave modified the milestones: 2.3.x, 2.3.8 Dec 19, 2020
mbhave added a commit that referenced this issue Dec 19, 2020
See gh-24205
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants