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

CLIENT:PAM: replace deprecated _pam_overwrite #7615

Closed

Conversation

alexey-tikhonov
Copy link
Member

with sss_erase_mem_securely()

And revert ef014b8.

@alexey-tikhonov alexey-tikhonov added the no-backport This should go to target branch only. label Sep 23, 2024
@alexey-tikhonov alexey-tikhonov marked this pull request as ready for review September 23, 2024 15:55
@alexey-tikhonov alexey-tikhonov linked an issue Sep 23, 2024 that may be closed by this pull request
@@ -50,6 +50,8 @@
#include "util/authtok-utils.h"
#include "util/dlinklist.h"

void sss_erase_mem_securely(void *p, size_t size); /* from memory_erase.c */
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is declared in the util.h header file. Redeclaring it here is not a good idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

sss_client code doesn't include 'util.h' (for a reason - to not pull any dependencies like 'talloc' etc)

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not have strong opinion on this, but what about moving the declaration to memory_erase.h?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can add it if you'd like but I personally don't like adding headers for the sake of one function declaration.

Copy link
Contributor

@aplopez aplopez Sep 25, 2024

Choose a reason for hiding this comment

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

I like Tomas' idea. This new header file can be included from util.h so that no other change is required, and included directly from the C files that use only this function. But do it as you feel it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, added a new header.

Copy link
Contributor

@thalman thalman left a comment

Choose a reason for hiding this comment

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

ACK, thanks

Copy link
Contributor

@aplopez aplopez left a comment

Choose a reason for hiding this comment

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

ACK.
Thanks.

Copy link
Contributor

@aplopez aplopez left a comment

Choose a reason for hiding this comment

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

Awesome. Thank you.

@alexey-tikhonov
Copy link
Member Author

alexey-tikhonov commented Sep 25, 2024

distcheck fails:

../../src/util/util.h:52:10: fatal error: util/memory_erase.h: No such file or directory
   52 | #include "util/memory_erase.h"

Copy link
Contributor

@thalman thalman left a comment

Choose a reason for hiding this comment

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

NACK - distcheck fails with this change

@alexey-tikhonov
Copy link
Member Author

Fixed.

Copy link
Contributor

@thalman thalman left a comment

Choose a reason for hiding this comment

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

ACK, thanks

@thalman
Copy link
Contributor

thalman commented Sep 25, 2024

@aplopez, do you want a second look?

@aplopez
Copy link
Contributor

aplopez commented Sep 26, 2024

@aplopez, do you want a second look?

LGTM.

@alexey-tikhonov alexey-tikhonov added the Ready to push Ready to push label Sep 26, 2024
@alexey-tikhonov
Copy link
Member Author

Pushed PR: #7615

  • master
    • 312e0eb - Revert "ci: allow deprecated functions during build"
    • 0330ebe - CLIENT:PAM: replace deprecated _pam_overwrite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport This should go to target branch only. Pushed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecated code used in 'sss_client/pam_sss.c'
3 participants