Skip to content

Conversation

@StefanHenke
Copy link
Contributor

No description provided.

| @sap/cds-dk | ^7 |
| Maven | 3.6.3 |
| SAP Cloud SDK | 5.10.0 |
| SAP BTP Spring Security Client | 3.5.1 |
Copy link
Contributor

@beckermarc beckermarc Jun 21, 2024

Choose a reason for hiding this comment

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

Is this mainly because of this TokenClaims constant that we are using now? :D
If yes we might want to rethink that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No :) 3.5.0 is not properly doing the auto-configuration for the proof-token check in hybrid scenarios where xsuaa and identity bindings are used. Also a CAP application would incorrectly enforce the proof-token check in App2App scenarios.
Both issues has been fixed with 3.5.1.

Cloud SDK 5.10.0 we could discuss. I changed it mainly due to the fact that CAP 2.10.0 is already compiling against 5.10.0. It includes two fixes for zero-trust which we will support soon. So, we wouldn´t need to increase minimum version again.

Copy link
Contributor

@beckermarc beckermarc Jun 21, 2024

Choose a reason for hiding this comment

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

I think those are two different aspects. For me a minimum required version means: "Minimum version for successful compilation of the code". That a newer version of some library contains bug fixes (in internal code) for some scenarios I think doesn't really justify a new minimum version in my opinion. That is rather a new "recommended version". As for example people using plain XSUAA are totally fine with sticking to an older version and are not required to update now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For 3.5.1 you could say it is about bug fixes. But at least 3.5.0 includes the whole proof token check feature. If we stick to minimum version 3 of the security library, it could be interpreted as a kind of a security issue if people are using IAS with < 3.5.0. Applications wouldn´t even recognize that incoming certificates are not validated. On the other hand you can say it is in the responsible of the applications to update dependencies regularly once available.

If we don´t increase the minimum version, I at least have to revert the code change to use the new constant in TokenClaims as this would lead to runtime exceptions in IAS scenarios.

Copy link
Contributor

@beckermarc beckermarc Jun 21, 2024

Choose a reason for hiding this comment

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

Maybe we should document then rather with the IAS-related features that >3.5.1 is recommended/required. My main reason why I am a little hesitant here is that 95% of our current stakeholders don't use IAS as of today (obviously). For those we would (unnecessarily) increase the minimum version. This might also include stakeholders that rely on SAP Java Buildpack, which might be slow to update to 3.5.1.

Here I don't see a good reason why we need to force the newest version on our stakeholders. Not saying, that I wouldn't always recommend using the latest version anyway. If they don't manage the XSUAA version themselves CAP will anyway take care of bringing along the 3.5.1 version (if not downgraded by buildpack or so).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Let´s do the following. I added a comment to the migration guide as part of PR #1047 that for proof of possession, 3.5.1 is needed. So, we can close this PR.

@chgeo chgeo deleted the min-version-sec-library branch February 4, 2025 11:09
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.

3 participants