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

Path access extension #2257

Open
wants to merge 24 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@Piankero
Contributor

Piankero commented Sep 26, 2018

Basics

Do not describe the purpose here but:

  • Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains *(my name)*)
    Please always add something to the the release notes.
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, should be in the commit messages.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

  • I added unit tests
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see doc/CODING.md)
  • meta data is updated (e.g. README.md of plugins)

@Piankero Piankero requested a review from markus2330 Oct 4, 2018

Piankero added some commits Oct 4, 2018

@Piankero

This comment has been minimized.

Show comment
Hide comment
@Piankero

Piankero Oct 4, 2018

Contributor

@sanssecours Did you ever have that problem with the "implicit function declaration" despite being declared in the header file in <unistd.h>? It's the reason why the build breaks. Honestly stackoverflow does not provide any useful help

Contributor

Piankero commented Oct 4, 2018

@sanssecours Did you ever have that problem with the "implicit function declaration" despite being declared in the header file in <unistd.h>? It's the reason why the build breaks. Honestly stackoverflow does not provide any useful help

@sanssecours

This comment has been minimized.

Show comment
Hide comment
@sanssecours

sanssecours Oct 4, 2018

Contributor

@sanssecours Did you ever have that problem with the "implicit function declaration" despite being declared in the header file in <unistd.h>?

As far as I can tell the POSIX version of unistd.h does not offer a function called euidaccess. Looks like the function is part of glibc though. If you want to use euidaccess on a system that uses the GNU C Library (Linux) you should define _GNU_SOURCE before you include the header unistd.h. At least that is how I read the documentation for the function.

Contributor

sanssecours commented Oct 4, 2018

@sanssecours Did you ever have that problem with the "implicit function declaration" despite being declared in the header file in <unistd.h>?

As far as I can tell the POSIX version of unistd.h does not offer a function called euidaccess. Looks like the function is part of glibc though. If you want to use euidaccess on a system that uses the GNU C Library (Linux) you should define _GNU_SOURCE before you include the header unistd.h. At least that is how I read the documentation for the function.

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Oct 5, 2018

Contributor

@ingwinlu can you maybe help out here?

Contributor

markus2330 commented Oct 5, 2018

@ingwinlu can you maybe help out here?

Show outdated Hide outdated .gitignore Outdated
@ingwinlu

This comment has been minimized.

Show comment
Hide comment
@ingwinlu

ingwinlu Oct 6, 2018

Contributor

glibc automatically exports all function as long as no feature macros are set. So I have no idea why it does not at least build on glibc tests.

You can use something like https://github.com/ElektraInitiative/libelektra/blob/master/src/plugins/passwd/CMakeLists.txt#L8 to test if the function is available in the current environment and change the build process accordingly.

Contributor

ingwinlu commented Oct 6, 2018

glibc automatically exports all function as long as no feature macros are set. So I have no idea why it does not at least build on glibc tests.

You can use something like https://github.com/ElektraInitiative/libelektra/blob/master/src/plugins/passwd/CMakeLists.txt#L8 to test if the function is available in the current environment and change the build process accordingly.

Piankero added some commits Oct 7, 2018

@Piankero

This comment has been minimized.

Show comment
Hide comment
@Piankero

Piankero Oct 7, 2018

Contributor

@sanssecours I included it and now the build process complains:

In file included from /home/jenkins/workspace/libelektra_PR-2257-5ERNEW5JQCBG6J7GJCVU5QCJS7S4QVA52KPOOT7HFF6AMH7HOMYQ/src/plugins/path/path.c:9:0:

/home/jenkins/workspace/libelektra_PR-2257-5ERNEW5JQCBG6J7GJCVU5QCJS7S4QVA52KPOOT7HFF6AMH7HOMYQ/src/plugins/path/path.h:12:0: error: "_GNU_SOURCE" redefined [-Werror]

 #define _GNU_SOURCE        // For euidaccess
Contributor

Piankero commented Oct 7, 2018

@sanssecours I included it and now the build process complains:

In file included from /home/jenkins/workspace/libelektra_PR-2257-5ERNEW5JQCBG6J7GJCVU5QCJS7S4QVA52KPOOT7HFF6AMH7HOMYQ/src/plugins/path/path.c:9:0:

/home/jenkins/workspace/libelektra_PR-2257-5ERNEW5JQCBG6J7GJCVU5QCJS7S4QVA52KPOOT7HFF6AMH7HOMYQ/src/plugins/path/path.h:12:0: error: "_GNU_SOURCE" redefined [-Werror]

 #define _GNU_SOURCE        // For euidaccess
@ingwinlu

This comment has been minimized.

Show comment
Hide comment
@ingwinlu

ingwinlu Oct 7, 2018

Contributor

add_definitions (-D_GNU_SOURCE) already adds it. you could add #ifndef or remove the #define line completely

Contributor

ingwinlu commented Oct 7, 2018

add_definitions (-D_GNU_SOURCE) already adds it. you could add #ifndef or remove the #define line completely

@sanssecours

This comment has been minimized.

Show comment
Hide comment
@sanssecours

sanssecours Oct 7, 2018

Contributor

@sanssecours I included it and now the build process complains:

If it the macro _GNU_SOURCE is already defined, then I guess that is not the problem.

Contributor

sanssecours commented Oct 7, 2018

@sanssecours I included it and now the build process complains:

If it the macro _GNU_SOURCE is already defined, then I guess that is not the problem.

@Piankero

This comment has been minimized.

Show comment
Hide comment
@Piankero

Piankero Oct 7, 2018

Contributor

Does the plugin work on your computer?

Yes it does. That's why I am confused that it does not even compile on the build system

Contributor

Piankero commented Oct 7, 2018

Does the plugin work on your computer?

Yes it does. That's why I am confused that it does not even compile on the build system

Piankero added some commits Oct 7, 2018

Show outdated Hide outdated .gitignore Outdated

Piankero added some commits Oct 7, 2018

@ingwinlu

This comment has been minimized.

Show comment
Hide comment
@ingwinlu

ingwinlu Oct 7, 2018

Contributor

well once you clean it up it should pass

reported during cmake:

-- Looking for euidaccess
-- Looking for euidaccess - found
-- Include Plugin path

no errors / warnings during compile on debian-stable.

Contributor

ingwinlu commented Oct 7, 2018

well once you clean it up it should pass

reported during cmake:

-- Looking for euidaccess
-- Looking for euidaccess - found
-- Include Plugin path

no errors / warnings during compile on debian-stable.

Show outdated Hide outdated src/plugins/path/path.c Outdated
Show outdated Hide outdated src/plugins/path/path.c Outdated
@markus2330

small things

@@ -1266,7 +1266,7 @@ ingroup:plugin
module:path
number:202
description:Could not change to user. You might want to try as root (use sudo !!).
description:Could not change to user.

This comment has been minimized.

@markus2330

markus2330 Oct 14, 2018

Contributor

Better to not mix unrelated changes into PRs.

@markus2330

markus2330 Oct 14, 2018

Contributor

Better to not mix unrelated changes into PRs.

This comment has been minimized.

@Piankero

Piankero Oct 15, 2018

Contributor

Why is this change unrelated? I created the error 202 and in order to reduce the number of errors I changed the text so I can reuse it for another error which is very similar but not worth distinguishable for the program (root -> user & user -> root changes are both the same problem but the old text above only includes root -> user)

@Piankero

Piankero Oct 15, 2018

Contributor

Why is this change unrelated? I created the error 202 and in order to reduce the number of errors I changed the text so I can reuse it for another error which is very similar but not worth distinguishable for the program (root -> user & user -> root changes are both the same problem but the old text above only includes root -> user)

This comment has been minimized.

@markus2330

markus2330 Oct 15, 2018

Contributor

Ok, so you just fixed a copy&paste error from an earlier commit, this was not clear from what I saw in GitHub.

@markus2330

markus2330 Oct 15, 2018

Contributor

Ok, so you just fixed a copy&paste error from an earlier commit, this was not clear from what I saw in GitHub.

Show outdated Hide outdated src/plugins/path/path.c Outdated
@markus2330

Further review.

Show outdated Hide outdated .gitignore Outdated
Show outdated Hide outdated .gitignore Outdated
status= idea
usedby/plugin= path
type= enum
r

This comment has been minimized.

@markus2330

markus2330 Oct 14, 2018

Contributor

formatting problem.

@markus2330

markus2330 Oct 14, 2018

Contributor

formatting problem.

@@ -341,7 +341,7 @@ module:path
number:57
description:could not stat file
severity:warning
severity:error

This comment has been minimized.

@markus2330

markus2330 Oct 14, 2018

Contributor

Why is this an error?

@markus2330

markus2330 Oct 14, 2018

Contributor

Why is this an error?

If `check/permission/types = [permission]` is also present it will check for the correct permissions
of the file/directory. Optionally, you can also add `check/permission/user = [user]"` which then checks the permissions
for the given user. When calling `kdb set` on the actual key, you have to run as `root` user
or the file permissions cannot be checked (you will receive an error message).

This comment has been minimized.

@markus2330

markus2330 Oct 14, 2018

Contributor

And how to avoid being root?

@markus2330

markus2330 Oct 14, 2018

Contributor

And how to avoid being root?

# ERROR:194
# Reason:
# Expected: User tomcat has [read/write] permission for the given file /var/log/application-file-restricted.log.
# Actual: User tomcat has [] permission for var/log/application-file-restricted.log.

This comment has been minimized.

@markus2330

markus2330 Oct 14, 2018

Contributor

No slash before /var?

@markus2330

markus2330 Oct 14, 2018

Contributor

No slash before /var?

char * lastCharDel (char * name)
{
int i = 0;
while (name[i] != '\0')

This comment has been minimized.

@markus2330

markus2330 Oct 14, 2018

Contributor

Isn't this strlen? (elektraStrLen)

@markus2330

markus2330 Oct 14, 2018

Contributor

Isn't this strlen? (elektraStrLen)

// The following code changes the egid if a group from a user matches the filegroup
// TODO: Whats a good default value for ngroups
// If the relevant group is ngroups+1 then it wont get recognized
int ngroups = 30;

This comment has been minimized.

@markus2330

markus2330 Oct 14, 2018

Contributor

Avoid this magic constant.

@markus2330

markus2330 Oct 14, 2018

Contributor

Avoid this magic constant.

This comment has been minimized.

@Piankero

Piankero Oct 15, 2018

Contributor

I tried to find another way around but the I did not find a way around the "magic constant".
getgrouplist requires a number in order to know how many groups to fetch.

Does anybody know a better way?

@Piankero

Piankero Oct 15, 2018

Contributor

I tried to find another way around but the I did not find a way around the "magic constant".
getgrouplist requires a number in order to know how many groups to fetch.

Does anybody know a better way?

Michael Zronek
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment