Alok, Implemented all of your suggestions and reissued webrev. Comments below.
Alok Aggarwal wrote: > On Thu, 29 Jan 2009, William Schumann wrote: > > >> Alok, >> Responses below. >> >> Alok Aggarwal wrote: >> >>> On Tue, 27 Jan 2009, William Schumann wrote: >>> >>> >>>> Passwords should be encrypted in manifests, not by the installer code. >>>> >>>> The basic fix was simple - remove encryption calls from the code - but >>>> it was noted that there could be XML entities encoded in the password, >>>> so XML entities will now be decoded. The XML decoding is basic and has >>>> some shortcuts, such as decoding only 1-byte numeric values. It is >>>> expected that the AI manifests will soon be restructured and then should >>>> be parsed with established parsers, such as in libxml2. >>>> >>>> Bugs fixed along the way: >>>> - values with spaces were being truncated at the first space >>>> - apostrophes used as value delimiters were not working >>>> - some typos with keyword name not matching intended storage variable >>>> name >>>> >>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=4246 >>>> http://cr.opensolaris.org/~wmsch/bug-4246/ >>>> >>> auto_parse.c: lines 581-583: What happens if there >>> are multiple blank spaces either one after the other >>> or separated by tokens? >>> >> Spaces are compressed to a single space - fixed. >> > > Okay. > > >>> auto_parse.c: decode_xml_entities(): This code looks >>> complicated. Given that the SC manifest is going to eventually move out to >>> SMF enhanced profiles, I do >>> wonder whether it's worth the effort to write this code >>> that's going to be thrown away once we move over to SMF >>> enhanced profiles. >>> >> 90 lines of code here. The effort would be in reviewing it and dealing with >> possible bugs. >> > > Maintainability is the other area that would need more > effort than usual. > Given that fully functional XML parsing code will be used in the near future, I think that supporting the use of XML elements in the SC manifest can wait until then. Removed decode_xml_entities() XML parsing code, filing it for possible use later. > auto_parse.c: line 627: I don't think we want to do > gratuitous truncation (say, if it's a password value). > A failure is more appropriate. > Added failure path for bad passwords. > auto_parse.c: line 667: shouldn't this be zero unless > the code didn't catch an error condition else where? > Traced bad errno to auto_parse_manifest.c where a malloc was given a length of -1 when optional XML element was missing. Fixed and removed nits. > auto_parse.c: line 691: I don't think this should get > logged to the console, logging to the install_log should > be adequate. > Removed. William > >> Another XML parsing problem is that it is assumed that the SC value is on >> the same line as the "propval" keyword and AI will fail if it isn't. >> >> Do you know a time frame for moving to SMF enhanced profiles? >> > > I don't know what the plans are in this area. > > >>> Lastly, which cases did you test while testing out these >>> changes? >>> >> Tried values: >> - single, multiple spaces >> - empty >> - all possible (5) XML entities; XML decimal, octal encodings >> - invalid XML entities, missing terminator >> - XML entities adjacent, at beginning of line, at end of line >> - value longer than VALUE_SIZE (added warning on overflow) >> > > Thanks. > > Alok > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >
