Limit input fields during new authz creation in sa.NewOrderAndAuthz (#6622)

A `core.Authorization` object has lots of fields (e.g. `status`, 
`attempted`, `attemptedAt`) which are not relevant to a 
newly-created authorization: a brand new authz can only be in 
the "pending" state, cannot have been attempted already or have 
been validated.

Fix a nil pointer dereference in `sa.NewOrderAndAuthzs` if a 
`req *sapb.NewOrderAndAuthzsRequest` is passed into the 
function with an inner nil `req.NewOrder`.

Add new tests. 
- TestNewOrderAndAuthzs_MissingInnerOrder 
  - Checks that
the nil pointer dereference no longer occurs 
- TestNewOrderAndAuthzs_NewAuthzExpectedFields 
  - Checks that the `Attempted`, `AttemptedAt`, `ValidationRecords`,
     and `ValidationErrors` fields for a brand new authz in the 
    `pending` state are correctly defaulted to `nil` in 
    `sa.NewOrderAndAuthzs`.

Add a new test assertion `AssertBoxedNil` that returns true for the
existence of a "boxed nil" - a nil value wrapped in a non-nil interface
type.

Fixes #6535

---------

Co-authored-by: Samantha <hello@entropy.cat>

sa/sa.go

@@ -312,6 +312,10 @@ func (ssa *SQLStorageAuthority) DeactivateAuthorization2(ctx context.Context, re
 // authorizations are created, but then their corresponding order is never
 // created, leading to "invisible" pending authorizations.
 func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb.NewOrderAndAuthzsRequest) (*corepb.Order, error) {
+	if req.NewOrder == nil {
+		return nil, errIncompleteRequest
+	}
+
 	output, err := db.WithTransaction(ctx, ssa.dbMap, func(txWithCtx db.Executor) (interface{}, error) {
 		// First, insert all of the new authorizations and record their IDs.
 		newAuthzIDs := make([]int64, 0)
@@ -333,14 +337,14 @@ func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb
 					am.IdentifierType,
 					am.IdentifierValue,
 					am.RegistrationID,
-					am.Status,
+					statusToUint[core.StatusPending],
 					am.Expires,
 					am.Challenges,
-					am.Attempted,
-					am.AttemptedAt,
+					nil,
+					nil,
 					am.Token,
-					am.ValidationError,
-					am.ValidationRecord,
+					nil,
+					nil,
 				})
 				if err != nil {
 					return nil, err

sa/sa_test.go

@@ -991,6 +991,99 @@ func TestNewOrderAndAuthzs(t *testing.T) {
 	test.AssertDeepEquals(t, names, []string{"com.a", "com.b", "com.c", "com.d"})
 }
 
+// TestNewOrderAndAuthzs_NonNilInnerOrder verifies that a nil
+// sapb.NewOrderAndAuthzsRequest NewOrder object returns an error.
+func TestNewOrderAndAuthzs_NonNilInnerOrder(t *testing.T) {
+	sa, fc, cleanup := initSA(t)
+	defer cleanup()
+
+	key, _ := jose.JSONWebKey{Key: &rsa.PublicKey{N: big.NewInt(1), E: 1}}.MarshalJSON()
+	initialIP, _ := net.ParseIP("17.17.17.17").MarshalText()
+	reg, err := sa.NewRegistration(ctx, &corepb.Registration{
+		Key:       key,
+		InitialIP: initialIP,
+	})
+	test.AssertNotError(t, err, "Couldn't create test registration")
+
+	_, err = sa.NewOrderAndAuthzs(context.Background(), &sapb.NewOrderAndAuthzsRequest{
+		NewAuthzs: []*corepb.Authorization{
+			{
+				Identifier:     "a.com",
+				RegistrationID: reg.Id,
+				Expires:        fc.Now().Add(2 * time.Hour).UnixNano(),
+				Status:         "pending",
+				Challenges:     []*corepb.Challenge{{Token: core.NewToken()}},
+			},
+		},
+	})
+	test.AssertErrorIs(t, err, errIncompleteRequest)
+}
+
+func TestNewOrderAndAuthzs_NewAuthzExpectedFields(t *testing.T) {
+	sa, fc, cleanup := initSA(t)
+	defer cleanup()
+
+	// Create a test registration to reference.
+	key, _ := jose.JSONWebKey{Key: &rsa.PublicKey{N: big.NewInt(1), E: 1}}.MarshalJSON()
+	initialIP, _ := net.ParseIP("17.17.17.17").MarshalText()
+	reg, err := sa.NewRegistration(ctx, &corepb.Registration{
+		Key:       key,
+		InitialIP: initialIP,
+	})
+	test.AssertNotError(t, err, "Couldn't create test registration")
+
+	expires := fc.Now().Add(time.Hour).UnixNano()
+	domain := "a.com"
+
+	// Create an authz that does not yet exist in the database with some invalid
+	// data smuggled in.
+	order, err := sa.NewOrderAndAuthzs(context.Background(), &sapb.NewOrderAndAuthzsRequest{
+		NewAuthzs: []*corepb.Authorization{
+			{
+				Identifier:     domain,
+				RegistrationID: reg.Id,
+				Expires:        expires,
+				Status:         string(core.StatusPending),
+				Challenges: []*corepb.Challenge{
+					{
+						Status: "real fake garbage data",
+						Token:  core.NewToken(),
+					},
+				},
+			},
+		},
+		NewOrder: &sapb.NewOrderRequest{
+			RegistrationID: reg.Id,
+			Expires:        expires,
+			Names:          []string{domain},
+		},
+	})
+	test.AssertNotError(t, err, "sa.NewOrderAndAuthzs failed")
+
+	// Safely get the authz for the order we created above.
+	obj, err := sa.dbReadOnlyMap.Get(authzModel{}, order.V2Authorizations[0])
+	test.AssertNotError(t, err, fmt.Sprintf("authorization %d not found", order.V2Authorizations[0]))
+
+	// To access the data stored in obj at compile time, we type assert obj
+	// into a pointer to an authzModel.
+	am, ok := obj.(*authzModel)
+	test.Assert(t, ok, "Could not type assert obj into authzModel")
+
+	// If we're making a brand new authz, it should have the pending status
+	// regardless of what incorrect status value was passed in during construction.
+	test.AssertEquals(t, am.Status, statusUint(core.StatusPending))
+
+	// Testing for the existence of these boxed nils is a definite break from
+	// our paradigm of avoiding passing around boxed nils whenever possible.
+	// However, the existence of these boxed nils in relation to this test is
+	// actually expected. If these tests fail, then a possible SA refactor or RA
+	// bug placed incorrect data into brand new authz input fields.
+	test.AssertBoxedNil(t, am.Attempted, "am.Attempted should be nil")
+	test.AssertBoxedNil(t, am.AttemptedAt, "am.AttemptedAt should be nil")
+	test.AssertBoxedNil(t, am.ValidationError, "am.ValidationError should be nil")
+	test.AssertBoxedNil(t, am.ValidationRecord, "am.ValidationRecord should be nil")
+}
+
 func TestSetOrderProcessing(t *testing.T) {
 	sa, fc, cleanup := initSA(t)
 	defer cleanup()

test/asserts.go

@@ -49,6 +49,22 @@ func AssertNotNil(t *testing.T, obj interface{}, message string) {
 	}
 }
 
+// AssertBoxedNil checks that an inner object is nil. This is intentional for
+// testing purposes only.
+func AssertBoxedNil(t *testing.T, obj interface{}, message string) {
+	t.Helper()
+	typ := reflect.TypeOf(obj).Kind()
+	switch typ {
+	// .IsNil() only works on chan, func, interface, map, pointer, and slice.
+	case reflect.Chan, reflect.Func, reflect.Interface, reflect.Map, reflect.Pointer, reflect.Slice:
+		if !reflect.ValueOf(obj).IsNil() {
+			t.Fatal(message)
+		}
+	default:
+		t.Fatalf("Cannot check type \"%s\". Needs to be of type chan, func, interface, map, pointer, or slice.", typ)
+	}
+}
+
 // AssertNotError checks that err is nil
 func AssertNotError(t *testing.T, err error, message string) {
 	t.Helper()