On 09/19/2012 02:42 PM, Peter Krempa wrote:
> The DAC security driver silently ignored errors when parsing the DAC
> label and used default values instead.
>
> With a domain containing the following label definition:
>
> <seclabel type='static' model='dac' relabel='yes'>
> <label>sdfklsdjlfjklsdjkl</label>
> </seclabel>
>
> the domain would start normaly but the disk images would be still owned
> by root and no error was displayed.
>
> This patch changes the behavior if the parsing of the label fails (note
> that a not present label is not a failure and in this case the default
> label should be used) the error isn't masked but is raised that causes
> the domain start to fail with a descriptive error message:
>
> virsh # start tr
> error: Failed to start domain tr
> error: internal error invalid argument: failed to parse DAC label
> 'sdfklsdjlfjklsdjkl' for domain 'tr'
>
> I also changed the error code to "invalid argument" from "internal
> error" and tweaked the various error messages to contain correct and
> useful information.
> ---
> Diff to v2:
> - Fixed all error reporting paths to contain useful messages
> - Tweaked error messages to contain more information
> - Fixed printing of seclabel->label instead of seclabel->imagelabel in the
> imagelabel parsing func.
>
> src/security/security_dac.c | 95
> +++++++++++++++++++++++++++++----------------
> 1 file changed, 61 insertions(+), 34 deletions(-)
>
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 211fb37..be65d6e 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -90,6 +90,7 @@ int parseIds(const char *label, uid_t *uidPtr, gid_t
> *gidPtr)
> return 0;
> }
>
> +/* returns 1 if label isn't found, 0 on success, -1 on error */
> static
> int virSecurityDACParseIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr)
> {
> @@ -98,20 +99,18 @@ int virSecurityDACParseIds(virDomainDefPtr def, uid_t
> *uidPtr, gid_t *gidPtr)
> virSecurityLabelDefPtr seclabel;
>
> if (def == NULL)
> - return -1;
> + return 1;
>
> seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
> if (seclabel == NULL || seclabel->label == NULL) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("security label for DAC not found in domain %s"),
> - def->name);
> - return -1;
> + VIR_DEBUG("DAC seclabel for domain '%s' wasn't found", def->name);
You use "seclabel" here, ...
> + return 1;
> }
>
> if (seclabel->label && parseIds(seclabel->label, &uid, &gid)) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("failed to parse uid and gid for DAC "
> - "security driver: %s"), seclabel->label);
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("failed to parse DAC label '%s' for domain '%s'"),
... "label" here ...
> + seclabel->label, def->name);
> return -1;
> }
>
> @@ -127,19 +126,35 @@ static
> int virSecurityDACGetIds(virDomainDefPtr def, virSecurityDACDataPtr priv,
> uid_t *uidPtr, gid_t *gidPtr)
> {
> - if (virSecurityDACParseIds(def, uidPtr, gidPtr) == 0)
> - return 0;
> + int ret;
>
> - if (priv) {
> - if (uidPtr)
> - *uidPtr = priv->user;
> - if (gidPtr)
> - *gidPtr = priv->group;
> - return 0;
> + if (!def && !priv) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Failed to determine default DAC label "
... and here ...
> + "for an unknown object"));
> + return -1;
> }
> - return -1;
> +
> + if ((ret = virSecurityDACParseIds(def, uidPtr, gidPtr)) <= 0)
> + return ret;
> +
> + if (!priv) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("DAC security label couldn't be determined "
..., but "security label" here.
> + "for domain '%s'"), def->name);
> + return -1;
> + }
> +
> + if (uidPtr)
> + *uidPtr = priv->user;
> + if (gidPtr)
> + *gidPtr = priv->group;
> +
> + return 0;
> }
>
> +
> +/* returns 1 if label isn't found, 0 on success, -1 on error */
> static
> int virSecurityDACParseImageIds(virDomainDefPtr def,
> uid_t *uidPtr, gid_t *gidPtr)
> @@ -149,21 +164,19 @@ int virSecurityDACParseImageIds(virDomainDefPtr def,
> virSecurityLabelDefPtr seclabel;
>
> if (def == NULL)
> - return -1;
> + return 1;
>
> seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
> if (seclabel == NULL || seclabel->imagelabel == NULL) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("security label for DAC not found in domain %s"),
> - def->name);
> - return -1;
> + VIR_DEBUG("DAC imagelabel for domain '%s' wasn't found", def->name);
> + return 1;
> }
>
> if (seclabel->imagelabel
> && parseIds(seclabel->imagelabel, &uid, &gid)) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("failed to parse uid and gid for DAC "
> - "security driver: %s"), seclabel->label);
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("failed to parse DAC imagelabel '%s' for domain
> '%s'"),
> + seclabel->imagelabel, def->name);
> return -1;
> }
>
> @@ -179,17 +192,31 @@ static
> int virSecurityDACGetImageIds(virDomainDefPtr def, virSecurityDACDataPtr
> priv,
> uid_t *uidPtr, gid_t *gidPtr)
> {
> - if (virSecurityDACParseImageIds(def, uidPtr, gidPtr) == 0)
> - return 0;
> + int ret;
>
> - if (priv) {
> - if (uidPtr)
> - *uidPtr = priv->user;
> - if (gidPtr)
> - *gidPtr = priv->group;
> - return 0;
> + if (!def && !priv) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Failed to determine default DAC imagelabel "
> + "for an unknown object"));
> + return -1;
> + }
> +
> + if ((ret = virSecurityDACParseImageIds(def, uidPtr, gidPtr)) <= 0)
> + return ret;
> +
> + if (!priv) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("DAC security imagelabel couldn't be determined "
And here you have "security imagelabel" as opposed to the other
"imagelabel"-only messages.
> + "for domain '%s'"), def->name);
> + return -1;
> }
> - return -1;
> +
> + if (uidPtr)
> + *uidPtr = priv->user;
> + if (gidPtr)
> + *gidPtr = priv->group;
> +
> + return 0;
> }
>
>
Code seems fine, though, so ACK with unifying those messages. I suggest
either "seclabel" and "imagelabel" OR 'security label" and "security
imagelabel", whatever you think suits the error and XML better, but one
for all errors with the same label.
Martin
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
