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

Clarify dependency policy of Sling bundles #149

Merged
merged 2 commits into from
Jan 4, 2024

Conversation

kwin
Copy link
Member

@kwin kwin commented Dec 6, 2023

This clarification was requested in the context of https://lists.apache.org/thread/wbcx13qlt5frkoz1p0jslwf0znqkb6h2.

Copy link
Contributor

@enapps-enorman enapps-enorman left a comment

Choose a reason for hiding this comment

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

The policy of depending on the lowest possible version is unnecessarily rigid. It creates a bunch of confusion for everyone who is using tooling to scan for security problems.

I hope you would reconsider changing this to be the lowest possible version that does not have known security vulnerabilities.

For me, that is the practical compromise solution that continues the goal of supporting a wide range of environments while also signaling that we care about (and are paying attention to) security vulnerabilities.

@kwin
Copy link
Member Author

kwin commented Dec 7, 2023

For me this clarification is primarily about setting the right expectation for our users. I don't think we would be able to timely do new releases in case e.g. a vulnerability in Commons IO or Commons Lang is detected which is used in hundreds of our bundles...

@enapps-enorman
Copy link
Contributor

e.g. a vulnerability in Commons IO or Commons Lang

commons-io has not had a known security vulnerability since version 2.6 (released on May 27, 2020)
commons-lang3 has not had any known security vulnerabilities at all

My impression is that people are using these guidelines as justification for not doing any work at all, or to prevent others from evolving the code to more modern dependencies.

For example, for SLING-12184 where you raised an objection about bumping up to a 5 year old newer dependency and remain on a 12 year old dependency!

@kwin
Copy link
Member Author

kwin commented Dec 7, 2023

Raising a dependency is totally fine if your require certain features or new API only available in a newer version. Just raising the dependencies to get rid of vulnerabilities is not useful (and sometimes might even prevent adoption by our users who know that they are not affected by a vulnerability and don't want to/cannot upgrade the dependent library

@enapps-enorman
Copy link
Contributor

enapps-enorman commented Dec 7, 2023

Just raising the dependencies to get rid of vulnerabilities is not useful!

That is just not true at all. There is value in not getting a bunch of false positives from security scanning tools and having to explain the reason every time! And some people won't take your word for it that the detected security warnings are no big deal and won't let your software in their door.

Bumping the dependencies to a newer version is trivial and only has to be done once in a while and the dependabot does most of the work for you anyways.

@kwin
Copy link
Member Author

kwin commented Dec 7, 2023

Obviously we don't agree here, so I would appreciate to hear other opinions @cziegeler and @rombert...

@reschke
Copy link
Contributor

reschke commented Dec 7, 2023

Throwing in my 2 cents as innocent bystander:

  • as a maintainer of an OSS library, I feel offended when people keep referencing an EOLd version of the library for no good reason
  • furthermore, if allowing to run/deploy with an ancient version is an excuse not to also test with a current version, something is very wrong (so the both "oldest compatible" and "latest and greatest" should be tested with)
  • the latter gets indeed messy when there are many permutations to test with

So by all means drop support for versions that have been officially EOLd.

@cziegeler
Copy link
Contributor

We all (open source projects, companies) get swamped with automatically generated security scan reports - these reports have no idea about Maven scopes, modularity, OSGi, you name it. The only way to get ahead of that flood is to update our dependencies to versions without known security vulnerabilities. If we don't do this, our modules look like they have security vulnerabilities and we do not care. It is pretty hard and time consuming to fight this with explanations. The only thing that works are actions - and that is updating the dependencies.

But we are also not doing a good service for our users if we allow them to use insecure dependencies. If we update our dependencies, we force our users to a more secure world.

However, updating a dependency in a pom to a newer version is usually a very cheap change. Releasing the module with the updated dependency is not that cheap anymore. At the same time, we most likely will always be behind with updating - or simply overlook a report for some time. Which means, our users still have to look for security vulnerabilities themselves and update their projects.

Taking all of this together, I think it is good to set a clear expectation to our users that they have to take care of their systems and update dependencies as required. That is the nature of a modular system.
On the other end, we should update dependencies with known vulnerabilities to a version that has no known vulnerabilities. But we need to clearly state that this only makes security scans happy. It does not fix anything anywhere.

@kwin
Copy link
Member Author

kwin commented Dec 9, 2023

So which changes of this PR do you guys consider useful or would you rather leave the site as is?

@rombert
Copy link
Contributor

rombert commented Dec 11, 2023

Obviously we don't agree here, so I would appreciate to hear other opinions @cziegeler and @rombert...

Tough question, I don't have a good answer for it, but I'm still thinking about it.

Trying to restate the problem as I see it: we have two "personas" that we would like to make life easy for:

  • developers working against less recent Sling-based platforms that would like to deploy new Sling bundles with minimal changes
  • developers whose projects are enrolled in security scans and that need spend time handling false positives stemming from security scans

When I started contributing to the Sling project, we only had the first group.

I think questioning the policy that we is good, because as the world changes we should not stay inflexible. I am not sure what the best answer is.

I think some good points have been mentioned:

  • we can't upgrade all dependencies all the time, we simply don't have the time for that
  • not all dependencies are equal. updates are needed only when a security dependency is reported

There are some questions still open for me:

How many dependency updates are security-sensitive?

if we were to embark on a policy of "update for security fixes only", would that make the updates level reasonable?

How many dependency updates actually affect a released product?

It might be that when updating a dependency version this version is already updated in all "relevant" third party products using this bundle. I am thinking about Sling CMS, Starter, AEM, Peregrine, Websight, etc.

We don't have that data today (and we never did) so we play on the safe side and say 'lowest possible'. Would it be possible to gather data (or ask for data contributions) so that we know that the minimum baseline is?

@cziegeler
Copy link
Contributor

Very good points. While it might be hard to get all that data, I would expect that installations out there are fairly up to date with security updates.
I would also like to add the question "how painful is it to update the required dependencies?" Since we started with the policy of lowest possible dependencies, a lot has changed. Especially tooling is far better these days. If someone wants to update a bundle, tooling will tell you before the deployment whether that bundle will resolve or needs additional things. Some tooling will also help you in finding these things. Based on that, it is probably less of a deal to update a Sling module together with it dependencies - of course as long as this does not require any code changes.

@@ -15,22 +15,21 @@ Maven provides projects with a nice feature called dependency management. In Sli

After working with this some time and trying to upgrade various dependencies we came to the conclusion, that using Maven dependency management is not going to work out in the Sling scenario.

Why ? Maven's dependency management is aimed at traditional applicaitons, which are glued together statically during the build process. For this environment, dependency management is a great thing, since it guarantees a consistent application setup.
Why? Maven's dependency management is aimed at traditional applications, which are glued together statically during the build process. For this environment, dependency management is a great thing, since it guarantees a consistent application setup.

Copy link
Contributor

Choose a reason for hiding this comment

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

This change looks good


In a dynamic application setup as provided by an OSGi framework the static dependency management of Maven does not help. Actually it even causes problematic results with respect to backwards compatibility when using the Maven Bundle Plugin.

Why's that ? The Maven Bundle Plugin constructs the bundle manifest and will generally automatically create the Import-Package header. If the providing library (from Maven's dependency list) has `Export-Package` headers with version numbers, the Maven Bundle Plugin will insert the respective version numbers for the `Import-Package` header. This makes perfect sense, because it is expected, that the artifact required at least the given package version.
Why's that? The Maven Bundle Plugin (or rather the underlying [Bnd library](https://bnd.bndtools.org/) constructs the bundle manifest and will generally automatically create the Import-Package header. If the providing library (from Maven's dependency list) has `Export-Package` headers with version numbers, the Maven Bundle Plugin will insert the respective version numbers for the `Import-Package` header. This makes perfect sense, because it is expected, that the artifact required **at least** the given package version.
Copy link
Contributor

Choose a reason for hiding this comment

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

This change looks good


When using Maven dependency management, upgrading any dependencies in the parent POM may automatically increase the version numbers in the `Import-Package` headers and hence may cause any such bundle to fail resolution if deployed - even though the bundle did not change and does not really require a new version of the dependency.

So, in the case of OSGi deployment, Maven's dependency management actually interferes with the OSGi framework dependency management.

As a consequence, I suggest we drop dependency management in the parent POM (almost) completely and state the following.
As a consequence, we dropped dependency management in the parent POM (almost) completely and state the following.
Copy link
Contributor

Choose a reason for hiding this comment

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

This change looks good


For details refer to the `pom.xml` of [sling-bundle-parent](https://github.com/apache/sling-parent/blob/master/sling-bundle-parent/pom.xml) and [sling](https://github.com/apache/sling-parent/blob/master/sling-parent/pom.xml).
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes look good


All dependencies per module are fully described in terms of version, scope, and classifier by the respective project.

The version of the module dependency should be selected according to the following rule: The lowest version providing the functionality required by the module (or bundle). By required functionality we bascially mean provided API.
The version of the module dependency should be selected according to the following rule: **The lowest version providing the functionality required by the module (or bundle)**. By required functionality we basically mean provided API.
Copy link
Contributor

Choose a reason for hiding this comment

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

This change looks good


Generally there is a constant flow of releases of dependent libraries. In general this should not cause the dependency version number of a using module to be increased. There is one exception though: If the fixed library version contains a bug fix, which has an influence on the operation of the module, an increase in the version number is indicated and should also be applied.


## References

* [Dependency Management](http://markmail.org/message/5qpmsukdk4mdacdy) -- Discussion thread about reducing Maven Dependency Management
* [Dependency Management](https://lists.apache.org/thread/gbx1t3kfcvqkoljb8mk7ymow94kn2m2o) -- Discussion thread about reducing Maven Dependency Management
* [SLING-811](https://issues.apache.org/jira/browse/SLING-811) -- The actual issue governing the changes to the project descriptors
Copy link
Contributor

Choose a reason for hiding this comment

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

This change looks good

In Sling OSGi bundles we have long had a policy of depending on the lowest possible version of a library/API, to ensure that our bundles are deployable in the widest possible range of environments. Therefore the responsibility of
ensuring that the environment is secure lies with the assembler and/or deployer of the application, which should make sure that the OSGi bundles they deploy are secure. As such, **we don't consider vulnerable dependencies of our bundles as security issues** by themselves. Usually the dependencies used by Sling [are semantically versioned](https://docs.osgi.org/whitepaper/semantic-versioning/index.html) and therefore security related version updates are fully binary backwards-compatible.
Further detail and some exceptions from that policy are outlined in [our wiki](https://cwiki.apache.org/confluence/display/SLING/Dependabot).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this change is ok as well as it does not say anything about not updating a dependency. It just provides additional important context

@cziegeler
Copy link
Contributor

I think the changes mentioned are good as it does not explicitly say that we have a policy to not update for security fixes. But it clarifies the important aspect that nevertheless our users must check for vulnerabilities in dependencies themselves.
So I'm fine with applying this change - if someone wants to add more to it, this can be done in a follow up PR

@kwin
Copy link
Member Author

kwin commented Dec 23, 2023

@enapps-enorman Can you propose changes for sections you don't agree with or are you fine with the remarks from @cziegeler?

@enapps-enorman
Copy link
Contributor

enapps-enorman commented Jan 3, 2024

@kwin

Can you propose changes for sections you don't agree with or are you fine with the remarks from @cziegeler?

The following is the only statement that I would like to see more clarity on since the "lowest version" doesn't consider known security vulnerabilities when choosing the version to depend on.

The version of the module dependency should be selected according to the following rule: The lowest version providing the functionality required by the module (or bundle). By required functionality we basically mean provided API.

For me, the rules for dependency version should prefer the oldest version that doesn't contain any known security vulnerability with guidelines like this:

  1. When the generated "Import-Package" clause does not materially change then it should be preferred to bump the dependency to this version. This would make the security scanning tools not flag this as problem and should not alter the binary compatibility at all.
  2. When the generated "Import-Package" clause does materially change, then bump the "minor segment" of the bundle version number and continue further development with this as the new minimum dependency version. Changing the "minor segment" of the bundle version leaves that previous minor bundle version namespace available for any fixes that need to be backported and released on the older branch. This should make it possible to support consumers who can't upgrade to the new version. This backport scenario would probably be rare so I would treat it as an "on demand" situation.

I would imagine that some of this decision could be assisted with a bnd plugin that could do some calculations against the baseline version of the bundle. Maybe some codified tooling that enforces the above and the other bundle version rules from https://sling.apache.org/documentation/development/version-policy.html#evolution-of-bundle-versions

@cziegeler
Copy link
Contributor

For me the key word is "should" - it doesn't prevent us from updating a dependency for whatever reason, being it a security bug or a regression or whatever. Once we try to come up with a policy that fits all use cases, we most likely will spent a lot of time on it, just to find out that we missed something. And even if we think we can do this perfect policy are we seriously thinking about having a policy police?

@enapps-enorman
Copy link
Contributor

For me the key word is "should" - it doesn't prevent us from updating a dependency for whatever reason, being it a security bug or a regression or whatever. Once we try to come up with a policy that fits all use cases, we most likely will spent a lot of time on it, just to find out that we missed something. And even if we think we can do this perfect policy are we seriously thinking about having a policy police?

Well, if you read my suggestion again you would see that it says "should" as well. It just tries to define some best practice guidance. If that is too complex for you, then go ahead and leave it ambiguous and continue to be afraid of changing anything.

@cziegeler
Copy link
Contributor

If we all agree that this is a "should" then I'm totally happy.

@kwin
Copy link
Member Author

kwin commented Jan 4, 2024

@enapps-enorman I added your remark in 86bc15d. Feel free to enhance in a follow-up PR. My main point is for consumers (i.e. they must not expect that all Maven dependencies are not having known vulnerabilities).

@kwin kwin merged commit e3ee973 into master Jan 4, 2024
1 check was pending
@kwin kwin deleted the feature/clarify-dependency-policy-of-bundles branch January 4, 2024 19:04
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.

None yet

5 participants