Re: [cobbler-devel] Bug introduced by 74bda5972fff (More refactoring around input validation.
On 02.09.2014 [08:25:12 -0700], Nishanth Aravamudan wrote: On 31.08.2014 [10:15:01 +0200], J?rgen Maas wrote: This is definitely a bug, i will take a look at it. FYI, the master branch is slated to become cobbler 3.0, this means functionality and API may (and shall) change or can be removed altogether (eg. s390/itanium). Yep, that's fine with me, to be honest. But one thing I'd like to see improved is the commit logs indicating what is functionally changing (even if just the intent). The logs right now are a bit terse and it takes looking at the actual commit to know what they do often :) One other example is the remote kickstart URL feature; this has been removed on purpose also as a result of multiple CVE reports. Kickstarts *must* reside on the Cobbler server and also they *must* be in the directory /var/lib/cobbler/kickstarts/ I wonder if it would be possible to provide a mirroring? That is, if the kickstart referenced is remote, can it not be mirrored over as part of the sync and thus be local for the deployment itself? I suppose if that's the case, the admin could just mirror them over. I do think there's one use-case that is no longer supported, but isn't a CVE -- users testing custom kickstarts. Presuming a good authorization model, regular users may not be allowed to edit the Cobbler-wide kickstarts. But they might want to test some custom partitioning, or feature flag, and would like to specify that in their kickstart file? FYI, found another bug introduced by this commit: remote.write_kickstart_template now calls file_path = validate.kickstart_file_path(file_path, for_item=False) but that does the following check: if not os.path.isfile(kickstart): raise CX(Invalid kickstart template file location %s, file not found % kickstart) which doesn't make a lot of sense for writing the kickstart from the webUI? (on creating a new one). -Nish ___ cobbler-devel mailing list cobbler-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/cobbler-devel
Re: [cobbler-devel] Bug introduced by 74bda5972fff (More refactoring around input validation.
On 05.09.2014 [15:00:45 -0700], Nishanth Aravamudan wrote: On 02.09.2014 [08:25:12 -0700], Nishanth Aravamudan wrote: On 31.08.2014 [10:15:01 +0200], J?rgen Maas wrote: This is definitely a bug, i will take a look at it. FYI, the master branch is slated to become cobbler 3.0, this means functionality and API may (and shall) change or can be removed altogether (eg. s390/itanium). Yep, that's fine with me, to be honest. But one thing I'd like to see improved is the commit logs indicating what is functionally changing (even if just the intent). The logs right now are a bit terse and it takes looking at the actual commit to know what they do often :) One other example is the remote kickstart URL feature; this has been removed on purpose also as a result of multiple CVE reports. Kickstarts *must* reside on the Cobbler server and also they *must* be in the directory /var/lib/cobbler/kickstarts/ I wonder if it would be possible to provide a mirroring? That is, if the kickstart referenced is remote, can it not be mirrored over as part of the sync and thus be local for the deployment itself? I suppose if that's the case, the admin could just mirror them over. I do think there's one use-case that is no longer supported, but isn't a CVE -- users testing custom kickstarts. Presuming a good authorization model, regular users may not be allowed to edit the Cobbler-wide kickstarts. But they might want to test some custom partitioning, or feature flag, and would like to specify that in their kickstart file? FYI, found another bug introduced by this commit: remote.write_kickstart_template now calls file_path = validate.kickstart_file_path(file_path, for_item=False) but that does the following check: if not os.path.isfile(kickstart): raise CX(Invalid kickstart template file location %s, file not found % kickstart) which doesn't make a lot of sense for writing the kickstart from the webUI? (on creating a new one). Something like the following? I'll send through a github pull request if it seems reasonable: diff --git a/cobbler/remote.py b/cobbler/remote.py index 835d29f..069e167 100644 --- a/cobbler/remote.py +++ b/cobbler/remote.py @@ -1991,7 +1991,7 @@ class CobblerXMLRPCInterface: what = write_kickstart_template self._log(what, name=file_path, token=token) self.check_access(token, what, file_path, True) -file_path = validate.kickstart_file_path(file_path, for_item=False) +file_path = validate.kickstart_file_path(file_path, for_item=False, write=True) try: utils.mkdir(os.path.dirname(file_path)) diff --git a/cobbler/validate.py b/cobbler/validate.py index ff63b8a..3727edf 100644 --- a/cobbler/validate.py +++ b/cobbler/validate.py @@ -84,7 +84,7 @@ def snippet_file_path(snippet): return snippet -def kickstart_file_path(kickstart, for_item=True): +def kickstart_file_path(kickstart, for_item=True, write=False): Validate the kickstart file path. @@ -113,7 +113,7 @@ def kickstart_file_path(kickstart, for_item=True): if not kickstart.startswith(KICKSTART_TEMPLATE_BASE_DIR): raise CX(Invalid kickstart template file location %s, it is not inside -if not os.path.isfile(kickstart): +if not os.path.isfile(kickstart) and not write: raise CX(Invalid kickstart template file location %s, file not found return kickstart ___ cobbler-devel mailing list cobbler-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/cobbler-devel
Re: [cobbler-devel] Bug introduced by 74bda5972fff (More refactoring around input validation.
On 31.08.2014 [10:15:01 +0200], J?rgen Maas wrote: This is definitely a bug, i will take a look at it. FYI, the master branch is slated to become cobbler 3.0, this means functionality and API may (and shall) change or can be removed altogether (eg. s390/itanium). Yep, that's fine with me, to be honest. But one thing I'd like to see improved is the commit logs indicating what is functionally changing (even if just the intent). The logs right now are a bit terse and it takes looking at the actual commit to know what they do often :) One other example is the remote kickstart URL feature; this has been removed on purpose also as a result of multiple CVE reports. Kickstarts *must* reside on the Cobbler server and also they *must* be in the directory /var/lib/cobbler/kickstarts/ I wonder if it would be possible to provide a mirroring? That is, if the kickstart referenced is remote, can it not be mirrored over as part of the sync and thus be local for the deployment itself? I suppose if that's the case, the admin could just mirror them over. I do think there's one use-case that is no longer supported, but isn't a CVE -- users testing custom kickstarts. Presuming a good authorization model, regular users may not be allowed to edit the Cobbler-wide kickstarts. But they might want to test some custom partitioning, or feature flag, and would like to specify that in their kickstart file? -Nish ___ cobbler-devel mailing list cobbler-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/cobbler-devel
Re: [cobbler-devel] Bug introduced by 74bda5972fff (More refactoring around input validation.
Well, this reminds me that using a combo is a regression as we are no longer able to provide a custom kickstart url (used to be a text field). On 08/28/2014 04:15 PM, Nishanth Aravamudan wrote: This commit (afaict, not bisected, as I'm trying to get my server up running) broke the web UI. It removed the inherit entry from the dropdown for systems' kickstarts. The default became (added in a separate commit) -- but then provided no means to set the kickstart as inherited again. The simple change below --- a/web/cobbler_web/views.py +++ b/web/cobbler_web/views.py @@ -1108,6 +1108,7 @@ def generic_edit(request, what=None, obj_name=None, editmode=new): # allow for an empty value in the webui kickstart_list = remote.get_kickstart_templates() kickstart_list.append() +kickstart_list.append(inherit) kickstart_list.sort() # populate some select boxes got me working again, but seems like a workaround possibly for something not expexpected. Just an FYI, these refactoring changes seem like they are also changing functionality -- dunno if that's ideal, it would be nice if the commits were a bit more split up. But I appreciate all the improvements I'm seeing! ___ cobbler-devel mailing list cobbler-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/cobbler-devel
