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

plugins: Plugin ideas added #2169

Closed
wants to merge 12 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@Piankero
Contributor

Piankero commented Aug 8, 2018

I want to propose 3 new plugin ideas. For the recursion plugin I will need some more advise though from more experienced programmers.

@Piankero Piankero requested a review from markus2330 Aug 8, 2018

@markus2330

Thank you, there are good ideas!

@@ -24,9 +25,45 @@ is allowed to occur for both device and mountpoint. When checking for
relative files, it is not enough to look at the first character if it is
a `/`, because remote file systems and some special names are valid, too.
If `check/permission = [permission], [user]` is also present it will check for the correct permissions

This comment has been minimized.

@markus2330

markus2330 Aug 8, 2018

Contributor

check/permissions also needs to be added to infos/metadata and doc/METADATA.ini

@markus2330

markus2330 Aug 8, 2018

Contributor

check/permissions also needs to be added to infos/metadata and doc/METADATA.ini

Show outdated Hide outdated src/plugins/path/README.md Outdated
Permissions available:
- `r`: **R**ead
- `w`: **W**rite
- `x`: e**X**ecute

This comment has been minimized.

@markus2330

markus2330 Aug 8, 2018

Contributor

I think you can fully implement this, missing are:

  • sticky bit
  • and then all of these permissions also for groups and others
@markus2330

markus2330 Aug 8, 2018

Contributor

I think you can fully implement this, missing are:

  • sticky bit
  • and then all of these permissions also for groups and others

This comment has been minimized.

@Piankero

Piankero Aug 8, 2018

Contributor

Alright, I will add it in the next commit!

@Piankero

Piankero Aug 8, 2018

Contributor

Alright, I will add it in the next commit!

This comment has been minimized.

@markus2330

markus2330 Aug 8, 2018

Contributor

If you agree and have/will fix it, a "thumbs up" 👍 or similar is enough.

@markus2330

markus2330 Aug 8, 2018

Contributor

If you agree and have/will fix it, a "thumbs up" 👍 or similar is enough.

This comment has been minimized.

@Piankero

Piankero Aug 8, 2018

Contributor

The sticky bit does not change the behavior of the program at all so I think it is better to ignore it.

I am thinking the same about groups. In the end its a a user who executes or uses the file in the setting. So probably its the best to keep it that simple and only provide rwx.

I am unclear now if you really want to split up permission and user as for me it is more intuitive to have them both in one metadata, eg. check/permission "rwx, java" (or maybe without the colon). If check/permission "rwx, " is given, the current user is taken?

@Piankero

Piankero Aug 8, 2018

Contributor

The sticky bit does not change the behavior of the program at all so I think it is better to ignore it.

I am thinking the same about groups. In the end its a a user who executes or uses the file in the setting. So probably its the best to keep it that simple and only provide rwx.

I am unclear now if you really want to split up permission and user as for me it is more intuitive to have them both in one metadata, eg. check/permission "rwx, java" (or maybe without the colon). If check/permission "rwx, " is given, the current user is taken?

This comment has been minimized.

@markus2330

markus2330 Aug 8, 2018

Contributor

In the implementation it matters: if the file does not exist, it is the sticky bit of the parent directory which may decide if the user can create the file.

Btw. did you check if there an "access (2)" which allows us to say for which user it should be checked? Then the implementation would be trivial.

Maybe there should also be a flag if the file already needs to be present. For log files there are many differences in this area. Actually, if the software does not allow the creation of files, we could simply create it.

I am unclear now if you really want to split up permission

Yes, the split is a must. Otherwise we get problems if the user names contains ",", spaces or similar.

@markus2330

markus2330 Aug 8, 2018

Contributor

In the implementation it matters: if the file does not exist, it is the sticky bit of the parent directory which may decide if the user can create the file.

Btw. did you check if there an "access (2)" which allows us to say for which user it should be checked? Then the implementation would be trivial.

Maybe there should also be a flag if the file already needs to be present. For log files there are many differences in this area. Actually, if the software does not allow the creation of files, we could simply create it.

I am unclear now if you really want to split up permission

Yes, the split is a must. Otherwise we get problems if the user names contains ",", spaces or similar.

This comment has been minimized.

@Piankero

Piankero Aug 8, 2018

Contributor

Btw. did you check if there an "access (2)" which allows us to say for which user it should be checked? Then the implementation would be trivial.

Unfortunately I do not understand what you mean by this 😕

Maybe there should also be a flag if the file already needs to be present. For log files there are many differences in this area.

Maybe we could add a check/path/exists setting which indicated if it has to exist already?

Actually, if the software does not allow the creation of files, we could simply create it.

The idea sound good but especially with logs there are rollovers and this could simply lead to a delayed configuration error which isn't wanted. I guess the user setting the specification should just give the executing user the creation (write) permission to the directory.

Yes, the split is a must. Otherwise we get problems if the user names contains ",", spaces or similar.

What about

  • check/permission = rw
  • check/permission/user = tomcat

?

@Piankero

Piankero Aug 8, 2018

Contributor

Btw. did you check if there an "access (2)" which allows us to say for which user it should be checked? Then the implementation would be trivial.

Unfortunately I do not understand what you mean by this 😕

Maybe there should also be a flag if the file already needs to be present. For log files there are many differences in this area.

Maybe we could add a check/path/exists setting which indicated if it has to exist already?

Actually, if the software does not allow the creation of files, we could simply create it.

The idea sound good but especially with logs there are rollovers and this could simply lead to a delayed configuration error which isn't wanted. I guess the user setting the specification should just give the executing user the creation (write) permission to the directory.

Yes, the split is a must. Otherwise we get problems if the user names contains ",", spaces or similar.

What about

  • check/permission = rw
  • check/permission/user = tomcat

?

This comment has been minimized.

@markus2330

markus2330 Aug 8, 2018

Contributor

Unfortunately I do not understand what you mean by this confused

I mean to use the syscall access (2) http://man7.org/linux/man-pages/man2/access.2.html

logs there are rollovers and this could simply lead to a delayed configuration error which isn't wanted.

Yes, this is certainly not wanted. I think we need to gather more data here for what applications do here.

One more advanced idea is that Elektra already opens a file and then passes /proc/self/fd/ instead of the real file name. Then it is impossible that the file disappears (but it needs additionally resources, namely an open file descriptor).

What about

Simply propose it in the PR. We can take a look tomorrow.

@markus2330

markus2330 Aug 8, 2018

Contributor

Unfortunately I do not understand what you mean by this confused

I mean to use the syscall access (2) http://man7.org/linux/man-pages/man2/access.2.html

logs there are rollovers and this could simply lead to a delayed configuration error which isn't wanted.

Yes, this is certainly not wanted. I think we need to gather more data here for what applications do here.

One more advanced idea is that Elektra already opens a file and then passes /proc/self/fd/ instead of the real file name. Then it is impossible that the file disappears (but it needs additionally resources, namely an open file descriptor).

What about

Simply propose it in the PR. We can take a look tomorrow.

- `r`: **R**ead
- `w`: **W**rite
- `x`: e**X**ecute

This comment has been minimized.

@markus2330

markus2330 Aug 8, 2018

Contributor

Checks for regular file/directory/symbolic link/... would be interesting, too. But they are only nice-to-have.

@markus2330

markus2330 Aug 8, 2018

Contributor

Checks for regular file/directory/symbolic link/... would be interesting, too. But they are only nice-to-have.

This comment has been minimized.

@Piankero

Piankero Aug 8, 2018

Contributor

I will leave this open for now and see in my thesis if it is needed that often. But the idea is good!

@Piankero

Piankero Aug 8, 2018

Contributor

I will leave this open for now and see in my thesis if it is needed that often. But the idea is good!

This comment has been minimized.

@markus2330

markus2330 Aug 8, 2018

Contributor

Yes, making everything complete is not a good idea anyway. It easily happens that stuff is added nobody ever needs. I just mentioned it so that you are aware that such a check might be relevant for some software.

@markus2330

markus2330 Aug 8, 2018

Contributor

Yes, making everything complete is not a good idea anyway. It easily happens that stuff is added nobody ever needs. I just mentioned it so that you are aware that such a check might be relevant for some software.

Show outdated Hide outdated src/plugins/path/README.md Outdated
Show outdated Hide outdated src/plugins/recursion/README.md Outdated
Show outdated Hide outdated src/plugins/recursion/README.md Outdated
Show outdated Hide outdated src/plugins/recursion/README.md Outdated
Show outdated Hide outdated src/plugins/recursion/README.md Outdated
Show outdated Hide outdated src/plugins/recursion/README.md Outdated
@markus2330

Most of the comments from the previous review are still not fixed.

Show outdated Hide outdated doc/METADATA.ini Outdated
@@ -1209,3 +1209,9 @@ severity:error
ingroup:plugin
module:typechecker
macro:PLUGIN_TYPECHECKER_SET
number:194
description:The given path does not meet the correct permissions

This comment has been minimized.

@markus2330

markus2330 Aug 9, 2018

Contributor

We should try to reuse/reduce the errors and make a nice classification system. Hopefully you can contribute to improve the error system.

@markus2330

markus2330 Aug 9, 2018

Contributor

We should try to reuse/reduce the errors and make a nice classification system. Hopefully you can contribute to improve the error system.

Show outdated Hide outdated src/plugins/network/README.md Outdated
Show outdated Hide outdated src/plugins/path/README.md Outdated
Show outdated Hide outdated src/plugins/path/README.md Outdated
Show outdated Hide outdated src/plugins/path/README.md Outdated
Show outdated Hide outdated src/plugins/recursion/README.md Outdated
Show outdated Hide outdated src/plugins/recursion/README.md Outdated
@markus2330

Thank you, much more clear now!

Show outdated Hide outdated src/plugins/recursion/README.md Outdated
Show outdated Hide outdated src/plugins/recursion/README.md Outdated
Show outdated Hide outdated src/plugins/recursion/README.md Outdated
Show outdated Hide outdated src/plugins/recursion/README.md Outdated
Show outdated Hide outdated src/plugins/recursion/README.md Outdated
Show outdated Hide outdated src/plugins/recursion/README.md Outdated
Show outdated Hide outdated src/plugins/recursion/README.md Outdated
Show outdated Hide outdated src/plugins/recursion/README.md Outdated
Show outdated Hide outdated src/plugins/recursion/README.md Outdated
Show outdated Hide outdated src/plugins/recursion/README.md Outdated
@Piankero

This comment has been minimized.

Show comment
Hide comment
@Piankero

Piankero Aug 11, 2018

Contributor

I updated the documentation.
On sunday I will

  1. Add src/error/specification for port & recursion
  2. write the correct shell recorder syntax

Concerning the "named" recursion it would be interesting to have your feedback.
Also on the Problem part in the recursion plugin readme which I updated.

Contributor

Piankero commented Aug 11, 2018

I updated the documentation.
On sunday I will

  1. Add src/error/specification for port & recursion
  2. write the correct shell recorder syntax

Concerning the "named" recursion it would be interesting to have your feedback.
Also on the Problem part in the recursion plugin readme which I updated.

[b]
```
which could cause an error since `a` cannot be loaded as `b` is yet non existent.

This comment has been minimized.

@markus2330

markus2330 Aug 11, 2018

Contributor

This order should not matter.

@markus2330

markus2330 Aug 11, 2018

Contributor

This order should not matter.

This comment has been minimized.

@Piankero

Piankero Aug 15, 2018

Contributor

Can you explain why? How exactly are keys loaded into the kdb?

I assume that it is loaded in the order of the file, i.e. it would be the same if I call kdb set ... in the same order as the config file

@Piankero

Piankero Aug 15, 2018

Contributor

Can you explain why? How exactly are keys loaded into the kdb?

I assume that it is loaded in the order of the file, i.e. it would be the same if I call kdb set ... in the same order as the config file

This comment has been minimized.

@markus2330

markus2330 Aug 15, 2018

Contributor

The plugins receive a whole keyset and can do whatever they want to do with it. So in the implementation you can lookup if something is present, it does not matter where in the keyset it is.

@markus2330

markus2330 Aug 15, 2018

Contributor

The plugins receive a whole keyset and can do whatever they want to do with it. So in the implementation you can lookup if something is present, it does not matter where in the keyset it is.

@Piankero

This comment has been minimized.

Show comment
Hide comment
@Piankero

Piankero Aug 16, 2018

Contributor

I redesigned the recursion plugin to only have the "named" approach now. The array approach might be confusing and isn't superior. Now we can easily adopt LCDproc's configuration

Contributor

Piankero commented Aug 16, 2018

I redesigned the recursion plugin to only have the "named" approach now. The array approach might be confusing and isn't superior. Now we can easily adopt LCDproc's configuration

@markus2330

Thank you. Quite some problems in error/specification and METADATA.ini.

Show outdated Hide outdated doc/METADATA.ini Outdated
[check/permission]
status= idea
usedby/plugin= path
type= enum

This comment has been minimized.

@markus2330

markus2330 Aug 16, 2018

Contributor

specify all cases.

@markus2330

markus2330 Aug 16, 2018

Contributor

specify all cases.

Show outdated Hide outdated doc/METADATA.ini Outdated
Show outdated Hide outdated doc/METADATA.ini Outdated
Show outdated Hide outdated doc/METADATA.ini Outdated
Show outdated Hide outdated src/error/specification Outdated
Show outdated Hide outdated src/error/specification Outdated
Show outdated Hide outdated src/plugins/network/README.md Outdated
Show outdated Hide outdated src/plugins/network/README.md Outdated
Show outdated Hide outdated src/plugins/recursion/README.md Outdated
Show outdated Hide outdated doc/METADATA.ini Outdated
Show outdated Hide outdated src/error/specification Outdated
Show outdated Hide outdated src/plugins/network/README.md Outdated
@Piankero

This comment has been minimized.

Show comment
Hide comment
@Piankero

Piankero Aug 18, 2018

Contributor

Hopefully now everything is accordingly.

Could you take a look at the new recursion plugin? It just contains a single check metadata now instead of 2.

Contributor

Piankero commented Aug 18, 2018

Hopefully now everything is accordingly.

Could you take a look at the new recursion plugin? It just contains a single check metadata now instead of 2.

@kodebach

This comment has been minimized.

Show comment
Hide comment
@kodebach

kodebach Sep 8, 2018

Contributor

@markus2330 I updated the recursion plugin README with results from a talk between me and @Piankero

I would also suggest renaming the recursion plugin to reference, because one major part of the work the proposed plugin would do is checking, whether references are valid. This functionality could also be useful elsewhere without the recursive part.

I propose:

  • changing check/recursion to check/reference
  • changing check/recursion/override to check/reference/override
  • changing check/recursion/restrict to check/reference/restrict
  • adding the boolean check/reference/recursive to enable/disable the check for loops in the reference graph

An example of such reference checking without recursion would be the references to driver configurations in the lcdproc server configuration.

I would also suggest either closing this PR entirely and opening separate PRs for the proposed plugins, or at least opening a separate PR for the recursion/reference plugin, to keep things a bit more organized.

Contributor

kodebach commented Sep 8, 2018

@markus2330 I updated the recursion plugin README with results from a talk between me and @Piankero

I would also suggest renaming the recursion plugin to reference, because one major part of the work the proposed plugin would do is checking, whether references are valid. This functionality could also be useful elsewhere without the recursive part.

I propose:

  • changing check/recursion to check/reference
  • changing check/recursion/override to check/reference/override
  • changing check/recursion/restrict to check/reference/restrict
  • adding the boolean check/reference/recursive to enable/disable the check for loops in the reference graph

An example of such reference checking without recursion would be the references to driver configurations in the lcdproc server configuration.

I would also suggest either closing this PR entirely and opening separate PRs for the proposed plugins, or at least opening a separate PR for the recursion/reference plugin, to keep things a bit more organized.

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Sep 8, 2018

Contributor

Yes, please let us open a new PR once we decided on the new name. If the checks are not limited to recursive checks, it makes sense to rename it. How can a user distinguish if a reference check is recursive or only a single check? What if we remove the old "struct" check and call the new plugin also "struct"? Is this plugin able to check some arbitrary recursive structure of configuration?

Btw. if possible we should avoid "override" because this already has a meaning within Elektra (it is a link to be preferred, see doc/METADATA.ini).

Contributor

markus2330 commented Sep 8, 2018

Yes, please let us open a new PR once we decided on the new name. If the checks are not limited to recursive checks, it makes sense to rename it. How can a user distinguish if a reference check is recursive or only a single check? What if we remove the old "struct" check and call the new plugin also "struct"? Is this plugin able to check some arbitrary recursive structure of configuration?

Btw. if possible we should avoid "override" because this already has a meaning within Elektra (it is a link to be preferred, see doc/METADATA.ini).

@kodebach

This comment has been minimized.

Show comment
Hide comment
@kodebach

kodebach Sep 9, 2018

Contributor

How can a user distinguish if a reference check is recursive or only a single check?

[/tests/keywithref]
check/reference = ref

[/tests/keywithrec]
check/reference = ref
check/reference/recursive = true

Here /tests/keywithref/ref would just be a single reference, while /tests/keywithrec/ref would be checked recursively.

An alternative would be the following:

[/tests/keywithref/ref]
check/reference = single

[/tests/keywithrec/ref]
check/reference = recursive

Here the type of check/reference would be enum, with the values single and recursive being supported. The name used for recursive checking would be the basename of the key with the check/recursive metadata. Then overriding the name could be done through a third enum value.

Personally I like the second option better. It makes more sense to me to specify the metadata on the actual reference key.

What if we remove the old "struct" check and call the new plugin also "struct"? Is this plugin able to check some arbitrary recursive structure of configuration?

The current proposed plugin would only check references. Checking the remaining structure could simply be done with the spec plugin:

[/tests/base/ref]
check/reference = recursive
check/reference/restrict = typeA

[/tests/base/typeA/_/name]
check/type = string
; etc

[/tests/base/typeA/_/size]
check/type = long
; etc

The basic idea is to restrict the references to a certain path X and then using X as a kind of type identifier. The direct sub-keys of X would then be 'instances' of X, so by using X/_/Y one can specify properties of the field Y of typeX.

Contributor

kodebach commented Sep 9, 2018

How can a user distinguish if a reference check is recursive or only a single check?

[/tests/keywithref]
check/reference = ref

[/tests/keywithrec]
check/reference = ref
check/reference/recursive = true

Here /tests/keywithref/ref would just be a single reference, while /tests/keywithrec/ref would be checked recursively.

An alternative would be the following:

[/tests/keywithref/ref]
check/reference = single

[/tests/keywithrec/ref]
check/reference = recursive

Here the type of check/reference would be enum, with the values single and recursive being supported. The name used for recursive checking would be the basename of the key with the check/recursive metadata. Then overriding the name could be done through a third enum value.

Personally I like the second option better. It makes more sense to me to specify the metadata on the actual reference key.

What if we remove the old "struct" check and call the new plugin also "struct"? Is this plugin able to check some arbitrary recursive structure of configuration?

The current proposed plugin would only check references. Checking the remaining structure could simply be done with the spec plugin:

[/tests/base/ref]
check/reference = recursive
check/reference/restrict = typeA

[/tests/base/typeA/_/name]
check/type = string
; etc

[/tests/base/typeA/_/size]
check/type = long
; etc

The basic idea is to restrict the references to a certain path X and then using X as a kind of type identifier. The direct sub-keys of X would then be 'instances' of X, so by using X/_/Y one can specify properties of the field Y of typeX.

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Sep 9, 2018

Contributor

Personally I like the second option better. It makes more sense to me to specify the metadata on the actual reference key.

Yes, I also like the second option better.

The basic idea is to restrict...

Good example. Could you please incorporate it into the README? But please continue using typeA (if I understood correctly it is changed to X later in the explanation).

Contributor

markus2330 commented Sep 9, 2018

Personally I like the second option better. It makes more sense to me to specify the metadata on the actual reference key.

Yes, I also like the second option better.

The basic idea is to restrict...

Good example. Could you please incorporate it into the README? But please continue using typeA (if I understood correctly it is changed to X later in the explanation).

@Piankero

This comment has been minimized.

Show comment
Hide comment
@Piankero

Piankero Sep 16, 2018

Contributor

@markus2330
There is a problem with the extension of the path plugin and the assumption if the metakeycheck/permission/user is not set, the current user is taken.

The problem lies in the euidaccess function and the need to change the egid.

Is is a bit difficult to explain:
*) euidaccess checks for the access with the egid and euid.
*) unfortunately there can only be one egid active at a time when calling euidaccess().
*) So if a user belongs to multiple groups and the filegroup is one of them, I have to actively switch to this group with setegid() before calling euidaccess()
*) To switch via setegid() I have to be running as root even though I am a member of the group I want to "switch" to.
*) When running as root though, the information is lost for which user you really want to check the permission for.

So it is absolutely required that check/permission/user and check/permission/types are both present.
I can only support to assume if check/permission/user is empty the root user is assumed. But checking for file access as root user is a bit fruitless.

I hope I could explain the problem so that it is understandable. Do you have a better solution or do we simply require the coexistence of both metadatas?

Contributor

Piankero commented Sep 16, 2018

@markus2330
There is a problem with the extension of the path plugin and the assumption if the metakeycheck/permission/user is not set, the current user is taken.

The problem lies in the euidaccess function and the need to change the egid.

Is is a bit difficult to explain:
*) euidaccess checks for the access with the egid and euid.
*) unfortunately there can only be one egid active at a time when calling euidaccess().
*) So if a user belongs to multiple groups and the filegroup is one of them, I have to actively switch to this group with setegid() before calling euidaccess()
*) To switch via setegid() I have to be running as root even though I am a member of the group I want to "switch" to.
*) When running as root though, the information is lost for which user you really want to check the permission for.

So it is absolutely required that check/permission/user and check/permission/types are both present.
I can only support to assume if check/permission/user is empty the root user is assumed. But checking for file access as root user is a bit fruitless.

I hope I could explain the problem so that it is understandable. Do you have a better solution or do we simply require the coexistence of both metadatas?

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Sep 21, 2018

Contributor

What is "check/permission/types"?

But checking for file access as root user is a bit fruitless.

I am not sure if it is fruitless. There are now many kernel extensions (and also file system features) to disallow root to do something. And I would prefer the "current user" and not root as default. And in the "current user" scenario no switching of egid is necessary.

If two options are absolutely required so that the plugin can do something useful it is of course possible to demand the presence of both options. Afaik this would be the first plugin, though.

Contributor

markus2330 commented Sep 21, 2018

What is "check/permission/types"?

But checking for file access as root user is a bit fruitless.

I am not sure if it is fruitless. There are now many kernel extensions (and also file system features) to disallow root to do something. And I would prefer the "current user" and not root as default. And in the "current user" scenario no switching of egid is necessary.

If two options are absolutely required so that the plugin can do something useful it is of course possible to demand the presence of both options. Afaik this would be the first plugin, though.

@Piankero

This comment has been minimized.

Show comment
Hide comment
@Piankero

Piankero Sep 22, 2018

Contributor

What is "check/permission/types"?

It is used for the actual permissions, e.g. "rwx"

And I would prefer the "current user" and not root as default. And in the "current user" scenario no switching of egid is necessary.

I wish for the same but when programming I could not circumvent the problem described above. But I have to switch egid since access only checks with one egid active. If you wish I could also demonstrate it on our next Elektra meeting.

Contributor

Piankero commented Sep 22, 2018

What is "check/permission/types"?

It is used for the actual permissions, e.g. "rwx"

And I would prefer the "current user" and not root as default. And in the "current user" scenario no switching of egid is necessary.

I wish for the same but when programming I could not circumvent the problem described above. But I have to switch egid since access only checks with one egid active. If you wish I could also demonstrate it on our next Elektra meeting.

@Piankero

This comment has been minimized.

Show comment
Hide comment
@Piankero

Piankero Oct 13, 2018

Contributor

All plugin ideas are now in implementation stage in their respective PR. Closing this PR

Contributor

Piankero commented Oct 13, 2018

All plugin ideas are now in implementation stage in their respective PR. Closing this PR

@Piankero Piankero closed this Oct 13, 2018

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