ncm-metaconfig: support running arbitrary commands in various steps #1451

stdweird · 2020-06-10T19:40:10Z

No description provided.

ned21 · 2020-06-10T20:01:34Z

ncm-metaconfig/src/main/perl/metaconfig.pm

+
+        my ($err, $out);
+        my %opts = (
+            shell => 1,


I think allowing shell=>1 is a really bad idea as it allows all sorts of security risks to be introduced by template admins. I'm OK with arbitrary commands from the profile but arbitrary shell expansions and injection attacks is probably a step too far. :)

@gombasg wanted filecopy powers 😉

what do you propose: we try to split the string in a command and run it without shell => 1. you know any good libraries that do that?

I guess the key question is: do we allow more than one command? If not, then make it a list and pass that through. If we want to allow multiple commands... but why? I am trying to remember what other (good) use cases we have for arbitrary commands in file copy. Mostly it's restarting daemons with custom scripts, which tbh should all be moving to systemd so it's a legacy use case.

Validation seems a valid use case for an arbitrary command, but is there a use use for multiple commands to execute validation? Seems unlikely? Probably best to wait for Gabor to chime in tomorrow but restricting this to the identified use cases to avoid abuse seems valid here.

@ned21 see also #1377 (comment) and comments below

Well, I've also found cases in our templates when errors from restarting/reloading the service needs to be suppressed, so we use service foo reload || true. A common reason is the initial build process, when the service is not expected to be operational yet, but we don't want the rest of the build process to fail. Changing every occurrence of restart to condrestart/try-restart could help in some cases, but systemd does not have try-reload. Hmm, if there was a component-level flag which could in turn tell CAF::Service to ignore all errors, then we could set that flag during the build...

Anyway, I'd like to repeat the idea of separating the definition of a command from triggering a command - @ned21, just like how startup actions work. So something like:

prefix "/software/components/metaconfig", "commands/dconf_update" = "dconf update"; "services/{/etc/dconf/db/gdm.d/00-login-screen}" = dict( "contents", ..., "actions/post", "dconf_update", );

This way, it would be more difficult to "hide" arbitrary commands in the noise, because all such commands would end up in a single block in the profile, suitable for reporting/auditing.

Just spotted this use of shell commands in our code base:

/usr/sbin/alternatives --set mta /usr/sbin/sendmail.sendmail; systemctl restart sendmail.service

But then with this code, I think we could still do that? The alternatives command would be run by the changed or post action, and then the restart would happen via the standard daemons attribute.

the daemons are only triggered by a change, you can also run the alternatives in the pre section.
when is it triggered now? only on change?

Yep, on change only via file copy.

in that case, yes this would work (but only because you are lucky enough that last command is a daemon action).

Right, but my point is that a lot of where we currently have shell "tricks" could be re-thought to be cleaner. But if we give people a $shell=>1 crutch they'll just port directly across from filecopy without trying to find a better way.

What we found with file copy is that once sites are using shell=1, there's now way to back out of it without risk of breakage. So it's much better to start with shell=0 and then only turn it on if we find a proper use case that can't be solved any other way.

stdweird · 2020-06-11T08:16:21Z

@gombasg i tried to add it. the naming is now a total mess far beyond my capabilties

ned21 · 2020-06-11T17:28:43Z

ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan

@@ -118,9 +130,26 @@ type ${project.artifactId}_config =  {
    'contents' : ${project.artifactId}_extension
    @{Predefined conversions from EDG::WP4::CCM::TextRender}
    'convert' ? ${project.artifactId}_textrender_convert
+    @{Actions (i.e. named registered commands) to run on pre, validation and/or post step.


Actions (i.e. names found /software/components/metadata/commands) to run when processing the service. Refer to the ${project.artifactId}_actions type definition for the available hooks for when a command may be run.

ned21 · 2020-06-11T17:32:50Z

ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan

 } = dict();

 type ${project.artifactId}_component = {
    include structure_component
    'services' : ${project.artifactId}_config{} with valid_absolute_file_paths(SELF)
+    @{Command registry for allowed actions, keys should be used as action value}
+    'commands' ? string{}


I think we need to say something about how commands are executed. i.e. (depending on discussion below): The commands will be executed via CAF::Process without shell.

ned21 · 2020-06-11T17:38:32Z

ncm-metaconfig/src/main/perl/metaconfig.pm

+
+        my ($err, $out);
+        my %opts = (
+            shell => 1,


I'd still vote for shell=>0. Looking at the docs, this should work just fine if we want the input to be strings?

my $cmd = CAF::Process->new(["ls -lh"], log => $self);

One of the reasons this structure is nice is that I can also nullify or replace any command I don't like with a central dict update:

'metaconfig/commands/not-for-me' = '/bin/true';

without having to loop through all services to find the right one.

I could potentially also do that conditionally using Aquilon's build status to e.g. avoid running commands that I know won't work during anaconda and but allowing them to run fine once we've left buildstatus=build.

(yeah, that's a bit ugly but potentially less so than the cmd||/bin/true hacks we have at the moment)

ned21 · 2020-06-12T12:44:32Z

This is a long thread so let me try summarise. I think we're pretty close to a conclusion:

I think we should use shell=0 rather than =1 because we can't disable it later.
I made a wording suggestion on the docs.
One of the points that Gabor made is that sometimes we don't care if a command fails (it's one of the drivers for shell syntax to allow ||/bin/true), or rather we don't want it to cause the component to "fail" and thus prevent any downstream dependencies from running. Should we support a flag, or a different hook since it's only applicable to changed and post, that is "run but don't error on failure"?

stdweird · 2020-06-12T12:54:35Z

@ned21

i'll look for a splitter, if you know one, please do share
for gabor's request: we can do something systemd style, and the command that starts with a - can fail without error.

other question: can we force that all commands start with a absolute path to the executable? otherwsie, i have to invent my own path resolution too.

stdweird · 2020-06-12T13:12:30Z

@ned21 i'll use use Text::ParseWords qw(old_shellwords) for the splitting (has el7 and el8 packages)

ned21 · 2020-06-12T14:44:01Z

i'll use use Text::ParseWords qw(old_shellwords) for the splitting (has el7 and el8 packages)

What about el6? :-( Do we need a splitter though? the CAF::Process doc says it accepts a string with shell=0. Or is that a mis-reading on my part?

for gabor's request: we can do something systemd style, and the command that starts with a - can fail without error.

I like that.

other question: can we force that all commands start with a absolute path to the executable? otherwsie, i have to invent my own path resolution too.

Yes, absolutely paths only.

stdweird · 2020-06-12T18:55:40Z

@ned21 for el6, i find no packages, and some sites seem to suggest it is part of perl 5.10 itself. can you check that? and where did you read that about CAF?

ned21 · 2020-06-12T19:51:31Z

Can't find any trace of Text-Parsewords on an EL6 system. However I tested:

use CAF::Process;
my $proc = CAF::Process->new(["ls /"]);
my $output = $proc->output();
$proc->execute();

Shows the contents of /.

my $proc = CAF::Process->new(["ls /d*"]);
gives: ls: cannot access /d*: No such file or directory.

So I think this confirms it's not necessary.

stdweird · 2020-06-12T20:15:10Z

@ned21 that splits on whitespace only, so no proper quote handling for args with space.
so perl -e 'use Text::ParseWords' fails on el6? what about repoquery --whatprovides "perl(Text::ParseWords)"

gombasg · 2020-06-12T20:39:09Z

On EL6, it is provided by “perl”. So if the Maven plugin uses the standard RPM macros to detect Perl dependencies, then everything should work. If not, then there’s a problem, because the dependency will need to be OS version specific – which needs to be implemented for RH8 support anyway… From: Stijn De Weirdt <[email protected]> Sent: 12 June 2020 22:15 To: quattor/configuration-modules-core <[email protected]> Cc: Gombas, Gabor (Enterprise Infrastructure) <[email protected]>; Mention <[email protected]> Subject: Re: [quattor/configuration-modules-core] ncm-metaconfig: support running arbitrary commands in various steps (#1451) @ned21<https://github.com/ned21> that splits on whitespace only, so no proper quote handling for args with space. so perl -e 'use Text::ParseWords' fails on el6? what about repoquery --whatprovides "perl(Text::ParseWords)" — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#1451 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABNYSMHBKLKHZI5JNVNL2N3RWKEFXANCNFSM4N2V7VTA>.

…

________________________________ NOTICE: Morgan Stanley is not acting as a municipal advisor and the opinions or views contained herein are not intended to be, and do not constitute, advice within the meaning of Section 975 of the Dodd-Frank Wall Street Reform and Consumer Protection Act. If you have received this communication in error, please destroy all electronic and paper copies and notify the sender immediately. Mistransmission is not intended to waive confidentiality or privilege. Morgan Stanley reserves the right, to the extent required and/or permitted under applicable law, to monitor electronic communications, including telephone calls with Morgan Stanley personnel. This message is subject to the Morgan Stanley General Disclaimers available at the following link: http://www.morganstanley.com/disclaimers. If you cannot access the links, please notify us by reply message and we will send the contents to you. By communicating with Morgan Stanley you acknowledge that you have read, understand and consent, (where applicable), to the foregoing and the Morgan Stanley General Disclaimers. You may have certain rights regarding the information that Morgan Stanley collects about you. Please see our Privacy Pledge https://www.morganstanley.com/privacy-pledge for more information about your rights.

stdweird · 2020-06-12T20:44:30Z

@gombasg only when the maven config has the dependecies explicitly set. otherwise, it's the usual perl.req script that adds them (in the perl(A::B::C) form).
can you open an issue about the el8 problems in the maven repo?

stdweird · 2020-06-12T21:29:44Z

@ned21 @gombasg i think i addressed the issues

ned21 · 2020-06-13T18:12:44Z

ah, yep: perl -e 'use Text::ParseWords' was a good call. I was looking in /usr/lib64/perl5/Text not /usr/share/perl5/Text/

ned21

Thanks for the changes. Just a few questions about docs, fixing some typos, etc.

ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan

ned21 · 2020-06-13T18:16:21Z

ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan

+type ${project.artifactId}_actions = {
+    @{Always run, happens before possible modifications}
+    'pre' ? string
+    @{Always run, happens before possible modifications, the file content is passed om stdin.


Suggested change

@{Always run, happens before possible modifications, the file content is passed om stdin.

@{Always run, happens before possible modifications, the new file content is passed on stdin.

Suggested change

@{Always run, happens before possible modifications, the file content is passed om stdin.

@{Always run before possible modifications with the new (or unchanged) file content is passed on stdin. A failure will cancel any file modification, unless the command is prefixed with -.

i find new (or unchanged) confusing. i would expect new/changed (or unchanged),but maybe i need some sleep ;)

this is actually the generated content, and this runs before anything is written. on failure, it just stops, writing to disk or even comparing with previous content is not attempted (i think, i would have to look at the code in FileWriter).

I was unsure about "unchanged", if it clouds the situation then let's just go for:

Always run before possible modifications with the generated file content is passed on stdin. A failure will cancel any file modification, unless the command is prefixed with -.

ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan

ncm-metaconfig/src/main/perl/metaconfig.pm

ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan

ned21 · 2020-06-14T19:21:50Z

ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan

+type ${project.artifactId}_actions = {
+    @{Always run, happens before possible modifications}
+    'pre' ? string
+    @{Always run, happens before possible modifications, the file content is passed om stdin.


Suggested change

@{Always run, happens before possible modifications, the file content is passed om stdin.

@{Always run before possible modifications with the new (or unchanged) file content is passed on stdin. A failure will cancel any file modification, unless the command is prefixed with -.

stdweird · 2020-06-14T20:19:24Z

ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan

@@ -86,6 +86,18 @@ type ${project.artifactId}_textrender_convert = {

 type caf_service_action = string with match(SELF, '^(restart|reload|stop_sleep_start)$');

+type ${project.artifactId}_actions = {
+    @{Always run, happens before possible modifications}


maybe also add A failure will cancel any file modification, unless the command is prefixed with -. here (because it's the case)?

Yes, good idea.

ned21

Unless I have missed something, I think my documentation suggestions are still pending?

stdweird · 2020-10-28T19:53:08Z

@ned21 please check again, i think it's ok now

stdweird · 2020-10-30T18:10:04Z

@ned21 changes pushed

Co-authored-by: Nathan Dimmock <[email protected]>

stdweird · 2020-11-03T13:41:11Z

@jrha conflicts resolved

ned21 · 2021-03-30T10:41:10Z

Anything else that's needed here? We are really keen to have #1452 which depends on this.

stdweird · 2021-03-30T11:01:12Z

@ned21 i'm happy with it 😉

stdweird requested a review from gombasg June 10, 2020 19:40

stdweird mentioned this pull request Jun 10, 2020

ncm-metaconfig: ssh daemon configuration support #1377

Closed

ned21 reviewed Jun 10, 2020

View reviewed changes

stdweird mentioned this pull request Jun 10, 2020

ncm-metaconfig: ssh daemon configuration support #1452

Merged

stdweird force-pushed the metaconfig_arbitrary_code_fc_styke branch from f725f72 to bad2f07 Compare June 11, 2020 08:41

ned21 reviewed Jun 11, 2020

View reviewed changes

stdweird mentioned this pull request Jun 12, 2020

ncm-download: support daemon actions on change #1454

Merged

stdweird force-pushed the metaconfig_arbitrary_code_fc_styke branch from 85e443c to 5ca3913 Compare June 12, 2020 21:36

ned21 reviewed Jun 13, 2020

View reviewed changes

ned21 reviewed Jun 14, 2020

View reviewed changes

stdweird commented Jun 14, 2020

View reviewed changes

jrha added this to the 21.3 milestone Oct 8, 2020

stdweird force-pushed the metaconfig_arbitrary_code_fc_styke branch from 5ca3913 to c4a029d Compare October 28, 2020 13:57

ned21 requested changes Oct 28, 2020

View reviewed changes

ned21 previously approved these changes Nov 2, 2020

View reviewed changes

ncm-filecopy: fix conditional reporter method call

3c14601

stdweird and others added 6 commits November 3, 2020 14:18

ncm-metaconfig: support running arbitrary commands in various steps

66b6600

ncm-metaconfig: setup command registry for running arbitrary commands

40cd477

ncm-metaconfig: switch to no-shell and support "ok to fail" commands

f29c07e

Update ncm-metaconfig/src/main/pan/components/metaconfig/schema.pan

da4aec9

Co-authored-by: Nathan Dimmock <[email protected]>

ncm-metaconfig: improve schema docstrings

69b4b7b

ncm-metaconfig: switch to shellwords for commandline splitting

1a391bc

stdweird dismissed ned21’s stale review via 1a391bc November 3, 2020 13:34

stdweird force-pushed the metaconfig_arbitrary_code_fc_styke branch from 51f7b3f to 1a391bc Compare November 3, 2020 13:34

jrha requested a review from ned21 November 5, 2020 16:11

ned21 approved these changes Mar 5, 2021

View reviewed changes

ned21 modified the milestones: 21.6, 21.3 Mar 5, 2021

jrha merged commit c0085ae into quattor:master Apr 13, 2021

	@{Always run, happens before possible modifications, the file content is passed om stdin.
	@{Always run, happens before possible modifications, the new file content is passed on stdin.

	@{Always run, happens before possible modifications, the file content is passed om stdin.
	@{Always run before possible modifications with the new (or unchanged) file content is passed on stdin. A failure will cancel any file modification, unless the command is prefixed with -.

ncm-metaconfig: support running arbitrary commands in various steps #1451

ncm-metaconfig: support running arbitrary commands in various steps #1451

Conversation

stdweird commented Jun 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stdweird commented Jun 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ned21 commented Jun 12, 2020 • edited Loading

stdweird commented Jun 12, 2020

stdweird commented Jun 12, 2020

ned21 commented Jun 12, 2020

stdweird commented Jun 12, 2020

ned21 commented Jun 12, 2020 • edited Loading

stdweird commented Jun 12, 2020

gombasg commented Jun 12, 2020 via email

stdweird commented Jun 12, 2020

stdweird commented Jun 12, 2020

ned21 commented Jun 13, 2020

ned21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ned21 left a comment

Choose a reason for hiding this comment

stdweird commented Oct 28, 2020

stdweird commented Oct 30, 2020

stdweird commented Nov 3, 2020

ned21 commented Mar 30, 2021

stdweird commented Mar 30, 2021

ned21 commented Jun 12, 2020 •

edited

Loading

ned21 commented Jun 12, 2020 •

edited

Loading