macadmins / nudge

A tool for encouraging the installation of macOS security updates.
Apache License 2.0
1.03k stars 187 forks source link

UTC vs Local Time #463

Closed boberito closed 2 months ago

boberito commented 1 year ago

Nudge uses UTC time which can make timing of things challenging when there are multiple offices across different time zones.

You either have to set the time when it isn't convenient for some or multiple profiles have to be set.

1) Is there a way to use local time? 2) What was the reasoning behind UTC? 3) Any interest in a PR for the ability to use local time?

aysiu commented 1 year ago

https://github.com/macadmins/nudge/issues/437 https://github.com/macadmins/nudge/issues/386

erikng commented 1 year ago
  1. No
  2. I prefer UTC/lazy when dealing with singular events. "multiple time zones" was out of scope at the time but we have other ways of handling this. Also I copied munki's method.
  3. Absolutely! I would prefer us to discuss what this looks like before you implement it.
boberito commented 1 year ago

I fully endorse the lazy when dealing with dates and times and zones and nonsense and copying munkis method is also good.

Slack good place?

erikng commented 1 year ago

We can do it here if you don't mind.

boberito commented 1 year ago

So I was thinking a config option under optionalFeatures like useLocalTime so people can opt in.

I see the line let dateFormat = "yyyy-MM-dd HH:mm:ss Z" and dateFormatterISO8601.timeZone = TimeZone(identifier: "UTC") in 2 places in Util.swift

I'm guessing those are the key. It may be as simple as throwing an if statement around that area, and if you want to use local time have system determine the local time zone and use that in the date format.

Thoughts?

aysiu commented 1 year ago

Not sure how difficult it would be to port to Nudge, but I think these are the relevant bits for how Munki handles things: https://github.com/munki/munki/blob/4e35f56b9c19d3dff3dc1bd85c634cbc1b507938/code/apps/Managed%20Software%20Center/Managed%20Software%20Center/MunkiItems.swift#L958-L959 https://github.com/munki/munki/blob/4e35f56b9c19d3dff3dc1bd85c634cbc1b507938/code/apps/Managed%20Software%20Center/Managed%20Software%20Center/munki.swift#L300-L308

erikng commented 1 year ago

What's unclear to me is what is the expected date an admin puts in? They can't put UTC now, so it needs to be a new date format.

Or are we saying continue to use the UTC date format but if an admin says 08:00:00 it means 8am locally and 20:00:00 means 8pm locally? If so that makes sense to me but may be confusing to others.

boberito commented 1 year ago

Yes! Completely that exactly.

boberito commented 1 year ago

I’m not sure how to not be confusing though. But if the admin enables the setting useLocalTime then they kind of should know what they’re getting into?

A thought could be change the default to always have the Z at the end and really stress it’s UTC. So the admin doesn’t have to enter it.

Or go the opposite, local time becomes the default and UTC is a choice.

aysiu commented 1 year ago

I believe, based the Munki code I posted above, that Munki just chops the z off the end, and then assumes local time. It's still in the XML, but the Munki documentation says local time is used.

erikng commented 1 year ago

I think keeping the time format identically as it is now and then have an optional key makes the most sense. It will be the least amount of code changes.

boberito commented 1 year ago

That’s probably very true. I’ll see if I can whip something together this weekend.

bradtchapman commented 1 year ago

Multinational companies might want to give their users the same deadline in their own local time, to be fair to all. We have employees that wrap the globe from Hawaii to Sydney. If we send an email announcing that users have "until 5pm Friday to apply this update," then the statement should be true for anyone.

erikng commented 1 year ago

Yes that's what he's proposing. I'm just trying to ensure we don't add a ton of complexity because time is used in other locations.

boberito commented 1 year ago

Well that was unexpected. Since a date in a configuration profile is an NSDate, it requires something like a Z at the end to be valid. Unless that's changed to a string, this idea may be dead.

erikng commented 1 year ago

Technically it accepts strings in the mobileconfig because JAMF does some really silly things but I've never been too public with that.

That said, why can't we keep the format and just convert to the local timezone?

boberito commented 1 year ago

I figured it would probably accept a string.

So if useLocalTime is set, it ignores the Z?

boberito commented 1 year ago

Another stumbling point. Swift only includes these TimeZone abbreviations. And well...there's quite a few more.

"BST": "Europe/London",
"UTC": "UTC",
"CAT": "Africa/Harare",
"WIT": "Asia/Jakarta",
"IST": "Asia/Kolkata",
"PET": "America/Lima",
"NZST": "Pacific/Auckland",
"AKST": "America/Juneau",
"MDT": "America/Denver",
"CST": "America/Chicago",
"ICT": "Asia/Bangkok",
"PST": "America/Los_Angeles",
"BRST": "America/Sao_Paulo",
"EAT": "Africa/Addis_Ababa",
"CEST": "Europe/Paris",
"NST": "America/St_Johns",
"WEST": "Europe/Lisbon",
"NDT": "America/St_Johns",
"PHT": "Asia/Manila",
"COT": "America/Bogota",
"EET": "Europe/Athens",
"MSK": "Europe/Moscow",
"JST": "Asia/Tokyo",
"ADT": "America/Halifax",
"TRT": "Europe/Istanbul",
"GST": "Asia/Dubai",
"NZDT": "Pacific/Auckland",
"CLT": "America/Santiago",
"CDT": "America/Chicago",
"CET": "Europe/Paris",
"BRT": "America/Sao_Paulo",
"EDT": "America/New_York",
"SGT": "Asia/Singapore",
"HST": "Pacific/Honolulu",
"EST": "America/New_York",
"WAT": "Africa/Lagos",
"HKT": "Asia/Hong_Kong",
"KST": "Asia/Seoul",
"CLST": "America/Santiago",
"GMT": "GMT",
"PDT": "America/Los_Angeles",
"BDT": "Asia/Dhaka",
"AKDT": "America/Juneau",
"MST": "America/Phoenix",
"IRST": "Asia/Tehran",
"WET": "Europe/Lisbon",
"AST": "America/Halifax",
"MSD": "Europe/Moscow",
"PKT": "Asia/Karachi",
"ART": "America/Argentina/Buenos_Aires",
"EEST": "Europe/Athens"

This would cover most major areas I believe, but using my obscure test..the country of Nauru is Pacific/Nauru and the abbreviation is NRT but swift doesn't include it. When I set my location to that in System Settings, it identifies as that timezone(Pacific/Nauru). And it looks like the abbreviation is needed for some ISO 8601 conversions.

erikng commented 1 year ago

Yay swift lol. This is why I hate dealing with dates and times.

So if useLocalTime is set, it ignores the Z?

That's what I would do to reduce code complexity. It then becomes a literal time to the local time in 24 hour time format.

bradtchapman commented 1 year ago

How does this affect your deferrals? It's still adding +launchdinterval, +3600, or +86400 to the last time the button was clicked, regardless of local time, correct?

And the requiredInstallationDate should not care about the UTC vs local time, it just needs to know "what time is this due."

Is that right?

erikng commented 1 year ago

Yes. IMO all this should impact is how the requiredInstallationDate is processed and normalized to the local time zone. All other logic should stay intact so as not to break everything else.

boberito commented 1 year ago

@erikng trying to figure out either a way to query some random thing like even Wikipedia to get that list live or even just get that actual list and make an include file of the couple hundred time zone abbreviations

erikng commented 1 year ago

Neither sound ideal. I don't want nudge calling out to endpoints outside of our control and a hardcoded list seems non ideal to me. Perhaps you are overthinking this.

Take the UTC time as a literal time and compare it against the local time in a 24 hour format. I'm not sure why you want to convert the UTC at all but perhaps I'm missing something you've discovered.

bradtchapman commented 1 year ago

So Swift doesn't check the definitions at /usr/share/zoneinfo ?

I realize not having Nauru is kind of an edge case, but does Swift fallback to another nearby location / time zone if you are in Nauru? Can you spoof your location? Or change it in System Preferences to simulate it?

boberito commented 1 year ago

@bradtchapman If you do a swift playground, change your location to Nauru in System Settings

let timeZone = TimeZone.current.identifier

print("Current: \(timeZone)")
let TZAD = TimeZone.abbreviationDictionary
let timeZoneIdentifiers = TimeZone.knownTimeZoneIdentifiers
print(timeZoneIdentifiers)

This prints out Current: Pacific/Nauru - you'll see Pacific/Nauru isnt listed. Nauru might be an edge case, but I'm sure there's less edgy cases not covered. Maybe UTC is the choice for them.

/var/db/timezone/tz/ and /usr/share/zoneinfo seem to have useful info, but it maybe looks more like the UTC offset - I'll have to dig deeper.

@erikng in order to convert UTC to local, you gotta do a dateFormatter.timeZone = TimeZone.current - but maybe I'm over complicating by converting back using ISO8601DateFormatter()

More keyboard banging will commence later on.

boberito commented 1 year ago
import Cocoa

let timeZone = TimeZone.current.identifier
print("Current: \(timeZone)")
let local = TimeZone.current.identifier
let localDate = Date()
print("UTC Time--",localDate)
let dateFormatter = DateFormatter()
dateFormatter.dateFormat = "yyyy-MM-dd HH:mm:ss Z"
dateFormatter.timeZone = TimeZone(abbreviation: "UTC")

var myDate = dateFormatter.string(from: localDate)
var convertedLocalTime = ""

if let dt = dateFormatter.date(from: myDate) {
    dateFormatter.timeZone = TimeZone.current
    dateFormatter.dateFormat = "yyyy-MM-dd H:mm:ss Z"
    convertedLocalTime = dateFormatter.string(from: dt)
} else {
    print("There was an error decoding the string")
}

print("convertedLocalTime String--",convertedLocalTime)
let isodateFormatter = ISO8601DateFormatter()
isodateFormatter.timeZone = TimeZone(identifier: local)
isodateFormatter.formatOptions = [.withFullDate, .withDashSeparatorInDate,.withSpaceBetweenDateAndTime,.withFullTime,.withColonSeparatorInTime]
let date = isodateFormatter.date(from:convertedLocalTime)
print("convertedLocalTime ISO--",date!)

Doing this in a playground with my local time results in

Current: America/New_York
UTC Time-- 2023-03-19 01:53:45 +0000
convertedLocalTime String-- 2023-03-18 21:53:45 -0400
convertedLocalTime ISO-- 2023-03-19 01:53:45 +0000

I believe things need to go back to an ISO formatted date.

According to https://developer.apple.com/documentation/foundation/dateformatter - Apple recommends

In macOS 10.12 and later or iOS 10 and later, use the ISO8601DateFormatter class when working with ISO 8601 date representations.

But as you can see convertedLocalTime ISO is not correct. I'm guessing it's due to the wrong format somehow.

erikng commented 1 year ago

Now try using different calendars other than Gregorian and watch the world blow up. :)

bradtchapman commented 1 year ago

"The requiredInstallationDate field supports digits and dashes only, and all deadlines must conform to the Gregorian calendar. Dates expressed using the Hebrew, Indian, Muslim, Chinese, or other lunar or quasi-lunar calendars will not be processed and Nudge will not launch."

You can't accommodate everyone. You'll make yourself nuts.

—Sent from my iPhone

On Mar 18, 2023, at 7:11 PM, Erik Gomez @.***> wrote:

 Now try using different calendars other than Gregorian and watch the world blow up. :)

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

boberito commented 1 year ago

Maybe this is dead until people much smarter than me can figure it out

import Cocoa

let localISOFormatter = ISO8601DateFormatter()
localISOFormatter.timeZone = TimeZone.current
let ISOstring = localISOFormatter.string(from: Date())
print("ISO String--",ISOstring)
let ISODate = localISOFormatter.date(from: ISOstring)
print("ISO Date--",ISODate!)

Outputs

ISO String-- 2023-03-19T14:41:42-04:00
ISO Date-- 2023-03-19 18:41:42 +0000

So maybe Date type is always UTC? 🤷🏻🤷🏻

boberito commented 1 year ago

I think I was going down the wrong path. I was trying to convert in Utils() the getCurrentDate and the other to nonUTC

But another of the problem is after doing research and talking to some people. Date type is really a double that’s EPOCH.

So unless the config profile was changed to a String (which you said it does accept) you can’t have a Date type in a configuration profile that’s not UTC. It’s just how it is.

I’m wondering if this may sort of already work in a way to use local time since you’ve done the hard work to allow it to accept a string that gets converted to a date.

More testing to occur tomorrow.

Apologies for the rambling in the Issue ticket, it’s been helpful as sort of notes. And it may be helpful for others to see how confusing a “simple” change could actually be.

boberito commented 1 year ago

OK! I have an answer @erikng. And the answer is...Nudge already does it!

When you use a String, it has to convert to a Date. And it by default uses the local time zone.

without Z in the date string
Desired 2023-03-19T14:40:00
PST UTC Required Installation Date: 2023-03-19 21:40:00 +0000
PST UTC Required Installation Date: 2023-03-19 18:40:00 +0000

with Z in the date string
Desired 2023-03-19T14:40:00
PST UTC Required Installation Date: 2023-03-19 21:40:00 +0000
EST UTC Required Installation Date: 2023-03-19 18:40:00 +0000

If I use a Date in the config profile

Desired 2023-03-19T14:40:00
PST UTC Required Installation Date: 2023-03-19 14:40:00 +0000
EST UTC Required Installation Date: 2023-03-19 14:40:00 +0000

So no pull request needed. Just maybe something added to the wiki, if you want to use local time, use a string.

erikng commented 1 year ago

Amazing lol. Good job old Erik!

bradtchapman commented 1 year ago

@boberito: can you update the wiki to explain what you found? Thanks!

kevinmcox commented 1 year ago

@bradtchapman I'll knock that out today.

kevinmcox commented 1 year ago

Wiki is updated: https://github.com/macadmins/nudge/wiki/osVersionRequirements#requiredinstallationdate---type-string-for-json-string-or-date-for-profile-default-value--required-yes

securitygeneration commented 1 year ago

Could the logic be updated to make JSON configs work the same? Yes, the JSON var is always going to be a string, but we could deal with that by making it such that Nudge accepts a timestamp with or without the Z. If it has a Z, deadline is in UTC time (i.e. same as the <date>); without it's in local time (i.e. same as <string>).

erikng commented 1 year ago

I'm going to leave this open until we can figure out local time on JSON.

bradtchapman commented 1 year ago

Didn't @boberito figure this out by dropping the "Z" off the end of the time stamp?

bradtchapman commented 7 months ago

Bump @erikng @boberito : I think this issue can be satisfied by users not adding "Z" to the end of the requiredInstallationDate. This has worked well for us. Thoughts?

erikng commented 2 months ago

Closing since I fixed the bug reported here: https://github.com/macadmins/nudge/issues/567

I've tested both types of configs and they are working as expected.