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

[libsmb2] update to v6.2 and add features to activate krb5 #42779

Merged
merged 3 commits into from
Jan 7, 2025

Conversation

michael-doubez
Copy link
Contributor

@michael-doubez michael-doubez commented Dec 18, 2024

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@michael-doubez
Copy link
Contributor Author

@microsoft-github-policy-service agree

@michael-doubez michael-doubez changed the title Libsmb2 update [libsmb2] update to v6.1 and add features to activate krb5 Dec 18, 2024
@jimwang118 jimwang118 added the category:port-update The issue is with a library, which is requesting update new revision label Dec 19, 2024
@michael-doubez michael-doubez changed the title [libsmb2] update to v6.1 and add features to activate krb5 [libsmb2] update to v6.2 and add features to activate krb5 Dec 23, 2024
Copy link
Contributor Author

@michael-doubez michael-doubez left a comment

Choose a reason for hiding this comment

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

LGTM

@michael-doubez
Copy link
Contributor Author

@jimwang118 Can you please review the PR ?

@jimwang118
Copy link
Contributor

All feature test passed with x64-windows triplet.

@jimwang118 jimwang118 added the info:reviewed Pull Request changes follow basic guidelines label Dec 24, 2024
ports/libsmb2/vcpkg.json Outdated Show resolved Hide resolved
@michael-doubez
Copy link
Contributor Author

@dg0yt krb5 transitive dependency added (thx for pointing that out)

ports/libsmb2/vcpkg.json Outdated Show resolved Hide resolved
@jimwang118 jimwang118 removed the info:reviewed Pull Request changes follow basic guidelines label Dec 25, 2024
@jimwang118 jimwang118 marked this pull request as draft December 25, 2024 02:04
@michael-doubez michael-doubez marked this pull request as ready for review December 25, 2024 08:05
@jimwang118 jimwang118 added the info:reviewed Pull Request changes follow basic guidelines label Dec 25, 2024
@michael-doubez
Copy link
Contributor Author

However, one may ask if these features should be default features at all, given vcpkg maintainers' today's reservation. Unless this is the only way to have reasonable security.

I respected the original package intent which was to include the feature by default.
I could have removed the gssapi default but IMHO it is more consistent to either include krb5 by default or not.

krb5 allows using smb with LDAP so it is a sensible default.
It is also smb2's default.

(In addition, I wonder if this could be generalized into a single "kerberos" feature, with platform-specific dependencies and configuration.)

Well, you are right. I suspect that gssapi is in fact part of krb5 but I don't have an ios env to test that so I prefered a /safe/ choice.

@michael-doubez
Copy link
Contributor Author

@dg0yt hello. Good for you ?

@dg0yt
Copy link
Contributor

dg0yt commented Dec 28, 2024

@dg0yt hello. Good for you ?

I still assume thatt a single "kerberos" feature would be more appropriate. But I don't want to push the idea. The PR has info:reviewed, and it is not broken.

@michael-doubez
Copy link
Contributor Author

I ll make the change to have only one feature.
Digging into libsmb2 library, it makes sense.

@michael-doubez
Copy link
Contributor Author

@dg0yt Here it is. Thank you for your feedback

Copy link
Contributor

@JavierMatosD JavierMatosD left a comment

Choose a reason for hiding this comment

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

Please address the default features comment

@JavierMatosD JavierMatosD marked this pull request as draft December 30, 2024 19:28
@michael-doubez
Copy link
Contributor Author

Please address the default features comment

@JavierMatosD done as suggested.

Although it means that on Linux people must be aware that feature must be activated if they want krb5 (LDAP ....) support.

Too bad there isn't an "exclude" feature or something like vcpkg install libsmb2[-krb5] because some features (like krb5 here) really make sense: they increase the range of supported protocol without modifying the interface.

@dg0yt
Copy link
Contributor

dg0yt commented Jan 1, 2025

I wonder if default features should really be stuck with outdated security capabilities.

ports/libsmb2/portfile.cmake Outdated Show resolved Hide resolved
michael-doubez and others added 2 commits January 1, 2025 23:28
@michael-doubez
Copy link
Contributor Author

@JavierMatosD Hello. Could you please review the PR ?

@dg0yt
Copy link
Contributor

dg0yt commented Jan 6, 2025

@michael-doubez You must mark as ready for review.

@michael-doubez michael-doubez marked this pull request as ready for review January 7, 2025 07:16
@JavierMatosD JavierMatosD merged commit 138e763 into microsoft:master Jan 7, 2025
17 checks passed
@michael-doubez michael-doubez deleted the libsmb2-update branch January 7, 2025 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants