-
Notifications
You must be signed in to change notification settings - Fork 27
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
Update ip address field to use IPAddressType to include ipv4 vs ipv6 data #193
Update ip address field to use IPAddressType to include ipv4 vs ipv6 data #193
Conversation
Modified the 'type' for the 'ip_address' element in an interface from 'xsd:string' to oval-sc:EntityItemIPAddressType'. This entity type already contains the needed 'datatype' attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I never looked at the original issue #134
I think we worked around the schema limitation in Joval by duplicating interfaces that had multiple addresses -- so you'd have multiple interface tags with the same values in their name sub-elements. I think this would be a breaking change for everyone.
A better idea, although still a breaking change, is the one you suggested, which would be to allow multiple ip_address elements.
I'm also not sure how I feel about using an Item type in the system_info. That feels wrong to me.
@solind what's your recommendation if using item type doesn't feel right? Do we clone that type and just call it something else? I'd also like your feedback on the need (or not) for max=unbounded? I'd like to wrap this up today if we can can finalize this. |
My recommendation would be to redefine InterfaceType to consist of a sequence of 4 elements:
I'd also add xs:restriction elements to specify the string format of the different address types (IP4, IP6 and MAC). See this stack overflow article. |
I'm always hesitant on regex implementations for IP addresses, but I'm trying this out with regexes from ipv4: ivp6; macaddress: |
Ignore my last commit, I was over-confident... and just tested it and I'm seeing errors I need to fix [ERROR]14:17:33 - Error validating CPE OVAL Results: C:/Program%20Files/SCAP%20Compliance%20Checker%205.10.1%20RC1/Resources/Schemas/oval_5.11.2/oval-system-characteristics-schema.xsd:156: Schemas parser error : element decl. '{http://oval.mitre.org/XMLSchema/oval-system-characteristics-5}ip_address', attribute 'type': The QName value '{http://www.w3.org/2001/XMLSchema}IPv4Type' does not resolve to a(n) type definition. |
@solind, I can fight my regex issues (the one above is just something dumb, but after fixing it, I ran into another round of regex issues). Given that other areas of the the schema ipv4 and ipv6 are just 'strings', I'm wondering restricting their values here has much value? I can make them strings, and be done in a couple minutes. Seeing the debates online of what is the 'best' or 'most accurate' regex for IPV4 or IPV6 makes me concerned about issues going forward. |
I've reverted the datatype back to xsd:string for now, out of concerns of issues going forward, vs the risks of leaving as string data. Given that other places in OVAL have just xsd:string for ipv4 and ipv6 data, I'm going for just fixing what this issue called for, allowing for both ipv4 and ipv6 to be present and valid |
@vanderpol I thought these XSD definitions looked pretty sweet and all-encompassing. |
But... if you're out of time, I'll try not to judge you too harshly if you just want to commit them as strings. |
Given everything else we have going on, and the benefits vs potential false positives, I'm just going to stick with strings and call 5.12 done. |
Modified the 'type' for the 'ip_address' element in an interface from 'xsd:string' to oval-sc:EntityItemIPAddressType'. This entity type already contains the needed 'datatype' attribute.
@solind, John @johnulmer-oval is out of the office for the week, and in my review of his update I'm wondering if he missed adding a maxoccurs=unbounded? Can you review this change, (and the original issue #134) to see if I should be adding that for this merge?